Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[exporter/azuremonitor] support sending to multiple azuremonitor exporter #36700

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

hgaol
Copy link
Member

@hgaol hgaol commented Dec 6, 2024

Description

currently, azuremonitor exporter doesn't support sending to multiple application insights. This PR will add this feature.

Link to tracking issue

Fixes #34188

Testing

Documentation

No additional configs.

@hgaol hgaol requested a review from a team as a code owner December 6, 2024 01:20
@hgaol hgaol requested a review from andrzej-stencel December 6, 2024 01:20
@github-actions github-actions bot requested a review from pcwiese December 6, 2024 01:20
@hgaol hgaol changed the title support sending to multiple azuremonitor exporter [exporter/azuremonitor] support sending to multiple azuremonitor exporter Dec 6, 2024
@hgaol
Copy link
Member Author

hgaol commented Dec 6, 2024

++ @pcwiese to help to review, thx!

@@ -63,7 +70,8 @@ func (f *factory) createTracesExporter(
return nil, errUnexpectedConfigurationType
}

tc, errInstrumentationKeyOrConnectionString := f.getTransportChannel(exporterConfig, set.Logger)
f.initLogger(set.Logger)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you cannot assume the same Logger is passed in for all. You might need to change this logic.

Copy link
Member Author

@hgaol hgaol Dec 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the careful review! Yes, it's better to use different logger, but the appinsights SDK only has one global diagnostics writer. If adding each logger to listener, there'll be dup messages. I don't know any elegant way to solve this issue at this point. any suggestion?

https://github.com/microsoft/ApplicationInsights-Go/blob/c063db145320a271b46e51eef7bceb88176b82c1/appinsights/diagnostics.go#L32C1-L33C52

// The one and only diagnostics writer.
var diagnosticsWriter = &diagnosticsMessageWriter{}

@atoulme
Copy link
Contributor

atoulme commented Dec 6, 2024

It feels like the factory should not handle any of the channel business. The component can be reused with the sharedcomponent lib, which feels more adequate.

@hgaol
Copy link
Member Author

hgaol commented Dec 8, 2024

It feels like the factory should not handle any of the channel business. The component can be reused with the sharedcomponent lib, which feels more adequate.

Currently in azuremonitorexporter, the metric, log, trace are separate exporters, and they're using the same transport channel. IMO, shared component is not suitable for such case. I plan to merge metric ,log, trace exporter into single one exporter and use shared component lib to cache it. Just like what fileexporter do. It may need more code changes as well as tests.

@hgaol
Copy link
Member Author

hgaol commented Dec 8, 2024

It feels like the factory should not handle any of the channel business. The component can be reused with the sharedcomponent lib, which feels more adequate.

Currently in azuremonitorexporter, the metric, log, trace are separate exporters, and for same component they're using the same transport channel. IMO, shared component is not suitable for such case. I plan to merge metric ,log, trace exporter into single one exporter and use shared component lib to cache it. Just like what fileexporter do. It needs more code changes as well as tests.

Hi @atoulme , I've done below changes and validated locally and it can ingest into multiple appinsights successfully. pls let me know if any comments, thanks for your careful review!

  • merged metric, log, trace exporters into AzureMonitorExporter, and deleted metric_exporter, log_exporter and trace_exporter.
  • used sharedcomponent to cache it by component id.
  • add start and shutdown to initialize and close channel.
  • adjusted tests.

@hgaol
Copy link
Member Author

hgaol commented Dec 9, 2024

Looks the failed checks are not caused by my change?

@hgaol hgaol requested a review from atoulme December 9, 2024 12:08
@hgaol hgaol force-pushed the 34188 branch 2 times, most recently from 36276b6 to 5ce5ef0 Compare December 18, 2024 15:18
Copy link
Contributor

github-actions bot commented Jan 2, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jan 2, 2025
@hgaol
Copy link
Member Author

hgaol commented Jan 2, 2025

gently ping, anyone can help reviewing this PR? thanks!

@github-actions github-actions bot removed the Stale label Jan 3, 2025
@hgaol
Copy link
Member Author

hgaol commented Jan 21, 2025

++ @pcwiese , could you help to review this enhancement PR? thanks!

@hgaol
Copy link
Member Author

hgaol commented Jan 22, 2025

I've validated using 2 Azure appinsights, and the new change is compatible with old ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[exporter/azuremonitor] collector can't send data to different azure application insights
3 participants