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

Downgrade System.Diagnostics.DiagnosticSource to v6 and add a check if OpenTelemetry instrumentation can be used #4487

Merged
merged 6 commits into from
Jan 10, 2024

Conversation

pmaytak
Copy link
Contributor

@pmaytak pmaytak commented Jan 4, 2024

Fixes #4456. An alternative to #4486 solution.

Changes proposed in this request

  • Downgrade System.Diagnostics.DiagnosticSource to v6, which is the same version as what Azure Functions runtime uses.
  • Catching error when instantiating Open Telemetry fails (means it's not available in that environment) and using null telemetry implementation instead.
  • See issue for more details.

Testing

  • Unit tests pass.
  • Tested with a dev app.
  • Also tested that Activity code from 5dc10b2 works.

@gladjohn
Copy link
Contributor

gladjohn commented Jan 6, 2024

thanks for referencing this issue, so even downgrading still causes some issues?

@pmaytak
Copy link
Contributor Author

pmaytak commented Jan 6, 2024

thanks for referencing this issue, so even downgrading still causes some issues?

Currently downgrading doesn't cause issues. However, it's not future-proof if we ever need newer APIs. Additionally, if the client app enables exporters, OpenTelemetry libraries 1.4.0 and above depend on DiagnosticSource 7.0.0+, so they'll have to use one of the workarounds anyway.

Directory.Packages.props Outdated Show resolved Hide resolved
@bgavrilMS
Copy link
Member

Have you thought about merging both PRs? So that it fixes the assembly load and fallbacks when Az Functions moves from 6 to 8 (?). Alternatively, this PR is enough and when Az Functions updates, the upgrade will break some folks. This should be a strong signal that they need to figure out a better solution. Thoughts?

@neha-bhargava
Copy link
Contributor

Were you able to test this with the exporter manually?

@pmaytak
Copy link
Contributor Author

pmaytak commented Jan 9, 2024

Have you thought about merging both PRs? So that it fixes the assembly load and fallbacks when Az Functions moves from 6 to 8 (?). Alternatively, this PR is enough and when Az Functions updates, the upgrade will break some folks. This should be a strong signal that they need to figure out a better solution. Thoughts?

Yea, I'll merge the PRs.

You mean when Azure Functions update their runtime to v8? I tested with a v5 library and everything works. So if they upgrade to v9 and we use v6/7, it's okay.

Were you able to test this with the exporter manually?

Yes, with the dev app. Workarounds still needed for in-process Function, but works otherwise.

* Adds a check if OTel can be used.

* Comments. More specific error type.
@pmaytak pmaytak changed the title Downgrade System.Diagnostics.DiagnosticSource to v6. Downgrade System.Diagnostics.DiagnosticSource to v6 and add a check if OpenTelemetry instrumentation can be used Jan 9, 2024
@pmaytak pmaytak enabled auto-merge (squash) January 9, 2024 03:01
@pmaytak pmaytak marked this pull request as draft January 9, 2024 03:01
auto-merge was automatically disabled January 9, 2024 03:01

Pull request was converted to draft

@pmaytak pmaytak marked this pull request as ready for review January 9, 2024 03:01
Copy link
Contributor

@neha-bhargava neha-bhargava left a comment

Choose a reason for hiding this comment

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

I did a quick test with MISE exporter too and logs look fine. Thanks!

@pmaytak pmaytak enabled auto-merge (squash) January 10, 2024 19:06
@pmaytak pmaytak merged commit ea376b4 into main Jan 10, 2024
6 checks passed
@pmaytak pmaytak deleted the pmaytak/azfunc-ds-6 branch January 10, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants