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

Updating Status based on the new spec #1313

Merged
merged 6 commits into from
Oct 13, 2020
Merged

Updating Status based on the new spec #1313

merged 6 commits into from
Oct 13, 2020

Conversation

eddynaka
Copy link
Contributor

Fixes #1311.

Changes

Please provide a brief description of the changes here.

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

@eddynaka eddynaka requested a review from a team September 29, 2020 00:26
@eddynaka
Copy link
Contributor Author

eddynaka commented Sep 29, 2020

What I did so far:

  • almost all status is mapped to Error
  • created a new enum StatusCode with Ok, Error, and Unset.

StatusCanonicalCode is still here, because we need that for grpc

@codecov
Copy link

codecov bot commented Sep 29, 2020

Codecov Report

Merging #1313 into master will decrease coverage by 0.22%.
The diff coverage is 92.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1313      +/-   ##
==========================================
- Coverage   77.88%   77.66%   -0.23%     
==========================================
  Files         226      226              
  Lines        6386     6314      -72     
==========================================
- Hits         4974     4904      -70     
+ Misses       1412     1410       -2     
Impacted Files Coverage Δ
...tation.AspNetCore/Implementation/HttpInListener.cs 87.50% <ø> (-0.12%) ⬇️
...ent/Implementation/GrpcClientDiagnosticListener.cs 83.33% <ø> (-0.46%) ⬇️
...tp/Implementation/HttpHandlerDiagnosticListener.cs 78.82% <25.00%> (ø)
src/OpenTelemetry.Api/Trace/ActivityExtensions.cs 99.12% <100.00%> (+0.03%) ⬆️
src/OpenTelemetry.Api/Trace/SpanHelper.cs 100.00% <100.00%> (+1.38%) ⬆️
src/OpenTelemetry.Api/Trace/Status.cs 100.00% <100.00%> (ø)
...metryProtocol/Implementation/ActivityExtensions.cs 90.34% <100.00%> (ø)
...try.Instrumentation.GrpcNetClient/GrpcTagHelper.cs 100.00% <100.00%> (ø)
...ient/Implementation/SqlClientDiagnosticListener.cs 75.36% <100.00%> (ø)
...mentation/RedisProfilerEntryToActivityConverter.cs 94.59% <100.00%> (ø)
... and 3 more

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM once the otlp.status_code is set to int.

@eddynaka
Copy link
Contributor Author

Just adding the label pr:do-not-merge because the spec is changing a little bit. So, waiting the formal changes to occur to finish it. Besides that, everything is done.

commenting OtlpTrace.Status.Type check

fixing test

undoing change to statuscanonicalcode

fixing tests

reiley's comments

SetStatus now saves enum instead of string

fixing changes after conversion

fixing redis tests

from http spec, if 1xx,2xx,3xx => unset, otherwise error.
from rpc spec, if ok => unset, otherwise error.

fixing sql tests

fixing redis tests
@eddynaka eddynaka self-assigned this Oct 12, 2020
@@ -121,51 +47,17 @@ public static Status ResolveSpanStatusForHttpStatusCode(int httpStatusCode)
/// <returns>Resolved span <see cref="Status"/> for the Grpc status code.</returns>
public static Status ResolveSpanStatusForGrpcStatusCode(int statusCode)
{
var newStatus = Status.Unknown;
var status = Status.Error;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not an expert in what gRPC is doing, but this seems off to me. Just took a quick look at the code it seems to set a status code in grpc.status_code, then it sets status from that, and then it removes grpc.status_code. This new version, I feel like we are going to lose some fidelity that was there previously. Maybe we should set status as it is done here, but leave grpc.status_code set to the raw "canonicalcode" value? Maybe we should also move StatusCanonicalCode into gRPC library since it is no longer in line with the spec.

/cc @alanwest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from this part, I'm still waiting a formal change in the spec to see.

I think I didn't change the part where it gets the value from the activity, so it might remain in the same way (the canonicalcode one). And, for the status itself, yeah, that will have only the 3 possibilities from spec.

Still waiting to confirm its behavior...

Copy link
Member

Choose a reason for hiding this comment

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

I guess most likely the gRPC code will be stored in a similar fashion as http.status_code since it has more semantic than the Unset/Ok/Error value.

I would suggest that we merge this PR as-is since the API part looks good, and we can catch the 0.7.0 release train.
The gRPC semantic convention is a lower priority comparing to the API since it is not a blocking issue, and it can be part of the 0.8.0 release or later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, GRPC already fills the status in the tag as grpc.status_code. So, it won't be a problem at all 👍

Yes, I think we can proceed and merge. Right now, the only part that is not "done" is the rpc part in the spec, since it will change because it was not updated in the first place.

Removing the label and adding to be merged asap

Copy link
Member

Choose a reason for hiding this comment

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

grpc.status_code is added by the library and the removal of the attribute in favor of the spec-approved attribute is intentional. See #1021.

Are we adding back grpc.status_code until the spec change gets sorted out?

I see a suggestion for something like rpc.grpc.status_code here open-telemetry/opentelemetry-specification#1044 (comment).

I agree with @CodeBlanch that it will likely make sense to bring the StatusCanonicalCode into the gRPC instrumentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @alanwest , it would be strange to remove one tag and add another tag which contains the same value...i would just enable it and try to set that from spec side. for example, so we would need to just enable it in our side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(just updated the issue to maintain grpc.status_code. With that, we will just have to remove the SetTag => null for that key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just enabled and i will create a new issue so we can follow the changes in the spec

@CodeBlanch CodeBlanch merged commit 378ba54 into open-telemetry:master Oct 13, 2020
@CodeBlanch CodeBlanch deleted the feature/breaking-status branch October 13, 2020 17:11
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.

Update Status to meet with the latest specification
4 participants