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

Change diagnostic-id spelling to comply with other languages #23141

Merged
merged 3 commits into from
Jul 24, 2021

Conversation

lmolkova
Copy link
Member

.NET (track1, 2), Java, and Python ServiceBus and EventHub SDKs spell Diagnostic-Id, and java uses diagnostic-id which causes context propagation between different languages to break. Azure/azure-sdk-for-net#22775

Since tracing support is still in preview (both with the plugin and through application insights agent) and we presumably don't have a lot of customers using it, making behavior-breaking change in Java before GA is more preffereable than making all other languages support diagnostic-id spelling.

@ghost ghost added the Azure.Core azure-core label Jul 22, 2021
@lmolkova lmolkova requested a review from samvaity July 22, 2021 17:57
Copy link
Member

@alzimmermsft alzimmermsft left a comment

Choose a reason for hiding this comment

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

Do any of the other names need to be changed as well? This may run into RevApi issues as a public property is changing value.

@lmolkova
Copy link
Member Author

Do any of the other names need to be changed as well?

nope, just this one, other names are not used beyond Java SDKs

This may run into RevApi issues as a public property is changing value.

Is there a way/review process to bypass it? Even though it's defined in azure-core, it's never used without tracing enabled

Yeah, this is what happened.

@lmolkova lmolkova enabled auto-merge (squash) July 22, 2021 19:04
@alzimmermsft
Copy link
Member

@lmolkova, this is achievable with a RevApi suppression, but before that let's see if @JonathanGiles and @srnagar are okay with this change.

@JonathanGiles
Copy link
Member

Make the change!

@srnagar
Copy link
Member

srnagar commented Jul 22, 2021

Given that this context is only used when tracing is enabled and the core-tracing library is still in preview, I am fine with making the change to this constant which actually fixes a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core azure-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants