Remove accessors for deprecated status code, fix receiver logic #2521
Conversation
bogdandrutu
force-pushed
the
rmsetter
branch
from
c7664b6 to
52164ac
Compare
Codecov Report
@@ Coverage Diff @@
## main #2521 +/- ##
=======================================
Coverage 91.73% 91.73%
=======================================
Files 265 265
Lines 15081 15082 +1
=======================================
+ Hits 13834 13836 +2
+ Misses 865 864 -1
Partials 382 382
Continue to review full report at Codecov.
|
bogdandrutu
force-pushed
the
rmsetter
branch
from
52164ac to
a85bb9c
Compare
| case otlptrace.Status_STATUS_CODE_OK: | ||
| // If status code is set then overwrites deprecated. | ||
| span.Status.DeprecatedCode = otlptrace.Status_DEPRECATED_STATUS_CODE_OK | ||
| case otlptrace.Status_STATUS_CODE_ERROR: | ||
| span.Status.DeprecatedCode = otlptrace.Status_DEPRECATED_STATUS_CODE_UNKNOWN_ERROR |
There was a problem hiding this comment.
Why these changes? The spec says that receivers MUST:
// If code!=STATUS_CODE_UNSET then the value of
deprecated_codeMUST be
// ignored, thecodefield is the sole carrier of the status.
This does not seem to match the spec anymore.
There was a problem hiding this comment.
What does the spec says? Why does "SetCode" updates deprecated but in this case we don't?
There was a problem hiding this comment.
Updated comments to SetCode as well as for the receiver to clarify when we are sender vs receiver.
There was a problem hiding this comment.
Discussed offline, so yes, OTLP receiver is both a "receiver" and "sender" in the sense that the proto uses these words here: https://github.com/open-telemetry/opentelemetry-proto/blob/59c488bfb8fb6d0458ad6425758b70259ff4a2bd/opentelemetry/proto/trace/v1/trace.proto#L239
The previous logic was to ignore deprecated if received non unset for new status code, but if downstream a service is not upgraded it should see the deprecated status set correctly. This change is to be consistent with `SetCode`. Signed-off-by: Bogdan Drutu <[email protected]>
bogdandrutu
force-pushed
the
rmsetter
branch
from
a85bb9c to
ceda5aa
Compare
Signed-off-by: Bogdan Drutu <[email protected]>
Updates #2488
The previous logic was to ignore deprecated if received non unset for new status code,
but if downstream a service is not upgraded it should see the deprecated status set correctly.
This change is to be consistent with
SetCode.Signed-off-by: Bogdan Drutu [email protected]