Clarify behavior for empty/not present/invalid trace_id and span_id fields #442
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
djaglowski
approved these changes
Jan 3, 2023
Aneurysm9
approved these changes
Jan 4, 2023
jmacd
reviewed
Jan 5, 2023
jmacd
reviewed
Jan 5, 2023
yurishkuro
reviewed
Jan 5, 2023
yurishkuro
reviewed
Jan 5, 2023
yurishkuro
reviewed
Jan 5, 2023
yurishkuro
approved these changes
Jan 5, 2023
|
Still, to me these statements do not belong in the specification anywhere; they look like accidents. Does the collector relaly implement this (if so where)? I would call this a defect covered by open-telemetry/opentelemetry-specification#1929. We are aware that there are ill-formed trace and span IDs, but we are referring to a |
jsuereth
reviewed
Jan 10, 2023
|
I did some spelunking and the "generate" recommendation comes from the very first commit: b5bcfff#diff-ef5f80fbf835dd57e14cb9264944f03d80cf6b04cc7671b0e7fb33167c67efcc where they were copied from Java repo, to which they were copied from OpenCensus open-telemetry/opentelemetry-java#134 and in OpenCensus the wording first time appeared here census-instrumentation/opencensus-proto#160 (authored by @SergeyKanzhelev, merged by @bogdandrutu ). I don't see any discussion on why "generating" is the right or wrong approach. @bogdandrutu @SergeyKanzhelev do you by any chance remember why this wording is used? |
|
I also checked the Collector's OTLP receiver and I don't see any code that implements the "generate on invalid" part. Unless we hear some arguments that explain why generating is important I agree to treat it as a bug and delete the requirement. |
tigrannajaryan
added a commit
to tigrannajaryan/opentelemetry-proto
that referenced
this pull request
Jan 10, 2023
…received This is considered an bug in the spec that was uncovered in the discussion here: open-telemetry#442 (comment) I did some spelunking and the "generate" recommendation comes from the very first commit: open-telemetry@b5bcfff#diff-ef5f80fbf835dd57e14cb9264944f03d80cf6b04cc7671b0e7fb33167c67efcc where they were copied from Java repo, to which they were copied from OpenCensus open-telemetry/opentelemetry-java#134 and in OpenCensus the wording first time appeared here census-instrumentation/opencensus-proto#160 (authored by @SergeyKanzhelev, merged by @bogdandrutu). We are deleting the requirement to generate a new trace id or span id if an invalid id is received. The receivers can decide how they want to treat the invalid id (just like upon receiving any other invalid id), e.g. they may drop it, log an error, accept the invalid data, etc. We are not going to prescribe a particular receiver behavior when invalid trace/span id is received.
|
I created a separate PR to delete the "generate" requirement: #444 If that gets accepted I will rewrite this PR as necessary. |
tigrannajaryan
added a commit
that referenced
this pull request
Jan 20, 2023
…received (#444) This is considered an bug in the spec that was uncovered in the discussion here: #442 (comment) I did some spelunking and the "generate" recommendation comes from the very first commit: b5bcfff#diff-ef5f80fbf835dd57e14cb9264944f03d80cf6b04cc7671b0e7fb33167c67efcc where they were copied from Java repo, to which they were copied from OpenCensus open-telemetry/opentelemetry-java#134 and in OpenCensus the wording first time appeared here census-instrumentation/opencensus-proto#160 (authored by @SergeyKanzhelev, merged by @bogdandrutu). We are deleting the requirement to generate a new trace id or span id if an invalid id is received. The receivers can decide how they want to treat the invalid id (just like upon receiving any other invalid id), e.g. they may drop it, log an error, accept the invalid data, etc. We are not going to prescribe a particular receiver behavior when invalid trace/span id is received.
tigrannajaryan
force-pushed
the
feature/tigran/clarify-not-present-id
branch
2 times, most recently
from
1a1ef77
to
1c6a5c8
Compare
…ields Resolves open-telemetry/opentelemetry-specification#3040 This is not a breaking change: - For Span it now defines more precisely the receiver behavior that was previously defined vaguely (e.g. it was unclear what "empty" means for bytes field). - For LogRecord it now defines the receiver behavior that was previously unspecified. This ensures that the wording are consistent with what we have for the Span.
tigrannajaryan
force-pushed
the
feature/tigran/clarify-not-present-id
branch
from
1c6a5c8
to
6e924c2
Compare
|
All, please take another look. #444 is now merged and this is rebased from main. |
Aneurysm9
approved these changes
Jan 20, 2023
mx-psi
approved these changes
Jan 23, 2023
carlosalberto
approved these changes
Jan 23, 2023
bogdandrutu
approved these changes
Jan 24, 2023
bogdandrutu
reviewed
Jan 24, 2023
akasyagroup
approved these changes
Jan 25, 2023
|
Has enough approvals, 2 days passed since last change, merging. |
|
Is it possible to have human readable documentation? Not everybody is .proto literate. There are many of us DevOps who are skilled in YAML only. |
@abitrolly The target audience of this repository are developers who implement OTLP protocol. The topics for devops engineers are here https://opentelemetry.io/ and if you would like something to be added there please create an issue at https://github.com/open-telemetry/opentelemetry.io/issues |
|
@tigrannajaryan missed the repo name. Opened a PR in open-telemetry/opentelemetry-specification#3152 to fill missing docs in protocol limitation. |
VinozzZ
pushed a commit
to VinozzZ/opentelemetry-proto
that referenced
this pull request
Jun 21, 2024
…received (open-telemetry#444) This is considered an bug in the spec that was uncovered in the discussion here: open-telemetry#442 (comment) I did some spelunking and the "generate" recommendation comes from the very first commit: open-telemetry@b5bcfff#diff-ef5f80fbf835dd57e14cb9264944f03d80cf6b04cc7671b0e7fb33167c67efcc where they were copied from Java repo, to which they were copied from OpenCensus open-telemetry/opentelemetry-java#134 and in OpenCensus the wording first time appeared here census-instrumentation/opencensus-proto#160 (authored by @SergeyKanzhelev, merged by @bogdandrutu). We are deleting the requirement to generate a new trace id or span id if an invalid id is received. The receivers can decide how they want to treat the invalid id (just like upon receiving any other invalid id), e.g. they may drop it, log an error, accept the invalid data, etc. We are not going to prescribe a particular receiver behavior when invalid trace/span id is received.
VinozzZ
pushed a commit
to VinozzZ/opentelemetry-proto
that referenced
this pull request
Jun 21, 2024
…ields (open-telemetry#442) Resolves open-telemetry/opentelemetry-specification#3040 This is not a breaking change: - For Span it now defines more precisely the receiver behavior that was previously defined vaguely (e.g. it was unclear what "empty" means for bytes field). - For LogRecord it now defines the receiver behavior that was previously unspecified. This ensures that the wording are consistent with what we have for the Span.
VinozzZ
pushed a commit
to VinozzZ/opentelemetry-proto
that referenced
this pull request
Jun 21, 2024
…received (open-telemetry#444) This is considered an bug in the spec that was uncovered in the discussion here: open-telemetry#442 (comment) I did some spelunking and the "generate" recommendation comes from the very first commit: open-telemetry@b5bcfff#diff-ef5f80fbf835dd57e14cb9264944f03d80cf6b04cc7671b0e7fb33167c67efcc where they were copied from Java repo, to which they were copied from OpenCensus open-telemetry/opentelemetry-java#134 and in OpenCensus the wording first time appeared here census-instrumentation/opencensus-proto#160 (authored by @SergeyKanzhelev, merged by @bogdandrutu). We are deleting the requirement to generate a new trace id or span id if an invalid id is received. The receivers can decide how they want to treat the invalid id (just like upon receiving any other invalid id), e.g. they may drop it, log an error, accept the invalid data, etc. We are not going to prescribe a particular receiver behavior when invalid trace/span id is received.
VinozzZ
pushed a commit
to VinozzZ/opentelemetry-proto
that referenced
this pull request
Jun 21, 2024
…ields (open-telemetry#442) Resolves open-telemetry/opentelemetry-specification#3040 This is not a breaking change: - For Span it now defines more precisely the receiver behavior that was previously defined vaguely (e.g. it was unclear what "empty" means for bytes field). - For LogRecord it now defines the receiver behavior that was previously unspecified. This ensures that the wording are consistent with what we have for the Span.
VinozzZ
pushed a commit
to VinozzZ/opentelemetry-proto
that referenced
this pull request
Jun 21, 2024
…received (open-telemetry#444) This is considered an bug in the spec that was uncovered in the discussion here: open-telemetry#442 (comment) I did some spelunking and the "generate" recommendation comes from the very first commit: open-telemetry@b5bcfff#diff-ef5f80fbf835dd57e14cb9264944f03d80cf6b04cc7671b0e7fb33167c67efcc where they were copied from Java repo, to which they were copied from OpenCensus open-telemetry/opentelemetry-java#134 and in OpenCensus the wording first time appeared here census-instrumentation/opencensus-proto#160 (authored by @SergeyKanzhelev, merged by @bogdandrutu). We are deleting the requirement to generate a new trace id or span id if an invalid id is received. The receivers can decide how they want to treat the invalid id (just like upon receiving any other invalid id), e.g. they may drop it, log an error, accept the invalid data, etc. We are not going to prescribe a particular receiver behavior when invalid trace/span id is received.
VinozzZ
pushed a commit
to VinozzZ/opentelemetry-proto
that referenced
this pull request
Jun 21, 2024
…ields (open-telemetry#442) Resolves open-telemetry/opentelemetry-specification#3040 This is not a breaking change: - For Span it now defines more precisely the receiver behavior that was previously defined vaguely (e.g. it was unclear what "empty" means for bytes field). - For LogRecord it now defines the receiver behavior that was previously unspecified. This ensures that the wording are consistent with what we have for the Span.
VinozzZ
pushed a commit
to VinozzZ/opentelemetry-proto
that referenced
this pull request
Jun 21, 2024
…received (open-telemetry#444) This is considered an bug in the spec that was uncovered in the discussion here: open-telemetry#442 (comment) I did some spelunking and the "generate" recommendation comes from the very first commit: open-telemetry@b5bcfff#diff-ef5f80fbf835dd57e14cb9264944f03d80cf6b04cc7671b0e7fb33167c67efcc where they were copied from Java repo, to which they were copied from OpenCensus open-telemetry/opentelemetry-java#134 and in OpenCensus the wording first time appeared here census-instrumentation/opencensus-proto#160 (authored by @SergeyKanzhelev, merged by @bogdandrutu). We are deleting the requirement to generate a new trace id or span id if an invalid id is received. The receivers can decide how they want to treat the invalid id (just like upon receiving any other invalid id), e.g. they may drop it, log an error, accept the invalid data, etc. We are not going to prescribe a particular receiver behavior when invalid trace/span id is received.
VinozzZ
pushed a commit
to VinozzZ/opentelemetry-proto
that referenced
this pull request
Jun 21, 2024
…ields (open-telemetry#442) Resolves open-telemetry/opentelemetry-specification#3040 This is not a breaking change: - For Span it now defines more precisely the receiver behavior that was previously defined vaguely (e.g. it was unclear what "empty" means for bytes field). - For LogRecord it now defines the receiver behavior that was previously unspecified. This ensures that the wording are consistent with what we have for the Span.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves open-telemetry/opentelemetry-specification#3040
This is not a breaking change:
For Span it now defines more precisely the receiver behavior that was previously defined vaguely (e.g. it was unclear what "empty" means for bytes field).
For LogRecord it now defines the receiver behavior that was previously unspecified. This ensures that the wording is consistent with what we have for the Span.