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

Add network change monitoring #176

Merged

Conversation

breedx-splk
Copy link
Contributor

The instrumentation that reports network changes has not yet been wired up. This adds it to the configuration and enables the instrumentation in the builder's build().

One additional aspect is that the CurrentNetworkProvider now has a setter on the OpenTelemetryRumBuilder. This allows existing components that may rely on this functionality to share the instance. The creation is lazy, so that if the builder user didn't create one, it is created at build time on demand.

I expect this CurrentNetworkProvider sharing to hopefully not be necessary in the long term, but right now there are existing disk buffering implementations (eg. Splunk's distro) that also need to determine information about the current network availability before attempting export.

@breedx-splk breedx-splk requested a review from a team December 8, 2023 00:44
Copy link
Contributor

@LikeTheSalad LikeTheSalad left a comment

Choose a reason for hiding this comment

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

Cool! I'm planning to add some changes in the future to the way the network is monitored due to the deprecated Google APIs issue... But in the meantime, I think it should be fine to use it as it is now.

@breedx-splk
Copy link
Contributor Author

Yeah, cool, long term I think we'll still need to:

  • Consider hiding the CurrentNetworkProvider and provide a more elegant way for users to get notified on otel-compatible network change events.
  • Consider if we need to check current network status before attempting to send from disk-buffered telemetry. Could cut down on failures/exceptions/unnecessary attempts and in worse case maybe throwing buffered data away.

@breedx-splk breedx-splk merged commit fac5ec1 into open-telemetry:main Dec 11, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants