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 support for Activity Status and StatusDescription in Zipkin Exporter #2572

Conversation

vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Nov 8, 2021

Partially Fixes #.
#2569

Changes

If (_activity.Status != ActivityStatusCode.Unset)
then: Activity.Status and Activity.StatusDescription will be used to set otel.status_code and error tags in Zipkin.
else custom tags (otel.status_code and otel.status_description) will be used (If set).

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Nov 8, 2021

Codecov Report

Merging #2572 (82a3f1d) into main (0ed4ed6) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2572      +/-   ##
==========================================
- Coverage   80.34%   80.31%   -0.03%     
==========================================
  Files         256      256              
  Lines        8752     8780      +28     
==========================================
+ Hits         7032     7052      +20     
- Misses       1720     1728       +8     
Impacted Files Coverage Δ
src/OpenTelemetry.Api/Internal/StatusHelper.cs 100.00% <100.00%> (ø)
...plementation/ZipkinActivityConversionExtensions.cs 99.08% <100.00%> (+0.15%) ⬆️
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs 86.53% <0.00%> (-5.77%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 74.28% <0.00%> (-2.86%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 97.65% <0.00%> (+0.78%) ⬆️

@vishweshbankwar vishweshbankwar changed the title Support Activity Status and StatusDescription for Zipkin Exporter Add support for Activity Status and StatusDescription in Zipkin Exporter Nov 9, 2021
@vishweshbankwar vishweshbankwar marked this pull request as ready for review November 9, 2021 23:49
@vishweshbankwar vishweshbankwar requested a review from a team November 9, 2021 23:49
// Starting version 6.0.0 in System.Diagnostic.DiagnosticSource
// Status and StatusDescription can be set using activity.SetStatus(ActivityStatusCode, string)
// Set otel.status_code and error to Status and StatusDescription respectively (If available)
if (activity.Status == ActivityStatusCode.Ok || activity.Status == ActivityStatusCode.Error)
Copy link
Member

Choose a reason for hiding this comment

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

Won't this break existing users relying on the extensions which set tags for status? I haven't had a chance to look at this fully yet, but I think we need a migration strategy. It might be possible to do that upstream so we don't have to modify all the existing exporters.

Copy link
Member Author

@vishweshbankwar vishweshbankwar Nov 10, 2021

Choose a reason for hiding this comment

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

it won't - we will still look up those tags in case Status and StatusDescription is not set. Could you please elaborate on "upstream change"?

Copy link
Member

Choose a reason for hiding this comment

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

Got it thanks.

The thought I have is, this is a lot of code and sort of tribal knowledge to expect every exporter to understand and do correctly. Ideally, we smooth that out so that they don't need to worry about it.

Some unknowns...

  • What do we do with this:

    public static void SetStatus(this Activity activity, Status status)

    Should it be updated to set activity.Status? I feel like yes.

  • What about people manually adding otel.status_code & otel.status_description? Right now we check for those and set error flags in zipkin and jaeger because we didn't have Activity.Status but if we change the extension method, should we continue to do that?

One random idea as an upstream example we could set otel.status_code and otel.status_descriptions tags from activity.Status/Description (or do the reverse) in a processor as a kind of migration helper people could decide to use or not use. Not saying it is the best option 😄

Basically I think we need to have a strategy for migrating to the new API. For Status and probably the .NET propagation stuff in .NET 6.

Copy link
Member

Choose a reason for hiding this comment

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

We can mark ActivityExtension method SetStatus as [Obsolete] with a message saying "Please use the native activity.SetStatus instead", right?

Copy link
Member Author

@vishweshbankwar vishweshbankwar Nov 10, 2021

Choose a reason for hiding this comment

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

Got it thanks.

The thought I have is, this is a lot of code and sort of tribal knowledge to expect every exporter to understand and do correctly. Ideally, we smooth that out so that they don't need to worry about it.

Some unknowns...

[VB] We cannot change the behavior of SetStatus() as it will be a breaking change then. The idea here is to keep extensions as is so anyone using it or continues to use it will not be affected. We will mark SetStatus from extensions obsolete and remove it in next major version change 2.x. I have noted some of these points in issue here - #2569.

  • What about people manually adding otel.status_code & otel.status_description? Right now we check for those and set error flags in zipkin and jaeger because we didn't have Activity.Status but if we change the extension method, should we continue to do that?

[VB] This should continue to work with this change. Anyone migrating to activity.SetStatus(ActivityStatusCode, desc) will also get correct status exported. Agree that exporters have to do extra work till we retire SetStatus(StatusCode)

One random idea as an upstream example we could set otel.status_code and otel.status_descriptions tags from activity.Status/Description (or do the reverse) in a processor as a kind of migration helper people could decide to use or not use. Not saying it is the best option 😄

[VB] In future we would want to move away from setting tags and only use Status and StatusDescription properties on Activity. Leave it to exporters how the status is exported.

Basically I think we need to have a strategy for migrating to the new API. For Status and probably the .NET propagation stuff in .NET 6.
@CodeBlanch - Please see my responses above inline [VB]. Can continue more discussion here #2569
cc: @cijothomas

Copy link
Member

Choose a reason for hiding this comment

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

Sorry @vishweshbankwar hadn't seen the issue! If we Obsolete the extension, the idea is in the future we would remove it and then also the logic looking for the tags in the exporters?

[VB] We cannot change the behavior of SetStatus() as it will be a breaking change then. The idea here is to keep extensions as is so anyone using it or continues to use it will not be affected.

I hear what you are saying, but I don't know if I 100% agree. If we changed it to chain to activity.Status/Description the API would be the same, only the implementation would change. And it is our prerogative to change our implementation details.

Anyway, I'm OK with the plan. Would still prefer to find a way to do this upstream. Normalize everything onto the Activity API so exporter authors only need to deal with that.

Copy link
Member

Choose a reason for hiding this comment

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

... but I think we need a migration strategy. It might be possible to do that upstream so we don't have to modify all the existing exporters.

+1, it'll be great if we can clarify the migration story/strategy (in the issue or PR description).

Copy link
Member

Choose a reason for hiding this comment

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

Would still prefer to find a way to do this upstream -- this'd be really nice!

Were you thinking something like
The SDK "checks status inside Tags, and based on that updates the Activity.Status, if not already set", so exporters can be modified to simply look Activity.Status?

Copy link
Member

Choose a reason for hiding this comment

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

@cijothomas

Were you thinking something like
The SDK "checks status inside Tags, and based on that updates the Activity.Status, if not already set", so exporters can be modified to simply look Activity.Status?

That was the first idea I had, ya. Inside the SDK somewhere, maybe a migration processor people could toggle as needed. What if we had a compatibility level switch somewhere?

Copy link
Member Author

@vishweshbankwar vishweshbankwar Nov 10, 2021

Choose a reason for hiding this comment

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

@cijothomas @CodeBlanch - Could you please help me understand the following:

How do we remove looking up tags for status in exporters - Wouldn't this be a breaking change in exporters?

If as a user I am using SetTag method to set status, Wouldn't I expect it to continue working without adding processor or switch?

Also, what is the end goal?
Do we want users to move away completely from using tags/SetStatus(StatusCode)?

statusCodeTagValue,
zipkinSpan.Tags.FirstOrDefault(t => t.Key == SpanAttributeConstants.StatusCodeKey).Value);

if (statusCodeTagValue == "unset")
Copy link
Member

Choose a reason for hiding this comment

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

this condition is never true, as this is part of inlinedata.

// Starting version 6.0.0 in System.Diagnostic.DiagnosticSource
// Status and StatusDescription can be set using activity.SetStatus(ActivityStatusCode, string)
// Set otel.status_code and error to Status and StatusDescription respectively (If available)
if (activity.Status == ActivityStatusCode.Ok || activity.Status == ActivityStatusCode.Error)
Copy link
Member

Choose a reason for hiding this comment

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

We can mark ActivityExtension method SetStatus as [Obsolete] with a message saying "Please use the native activity.SetStatus instead", right?

// Starting version 6.0.0 in System.Diagnostic.DiagnosticSource
// Status and StatusDescription can be set using activity.SetStatus(ActivityStatusCode, string)
// Set otel.status_code and error to Status and StatusDescription respectively (If available)
if (activity.Status == ActivityStatusCode.Ok || activity.Status == ActivityStatusCode.Error)
Copy link
Member

Choose a reason for hiding this comment

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

Would still prefer to find a way to do this upstream -- this'd be really nice!

Were you thinking something like
The SDK "checks status inside Tags, and based on that updates the Activity.Status, if not already set", so exporters can be modified to simply look Activity.Status?

@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jan 28, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2022

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants