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

Declare OTLP/JSON Stable #2930

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan
Copy link
Member Author

We need a lot of eyes on this before we merge this.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Would like to clarify the behavior for invalid (all 0) trace/span IDs. In other words we have 3 cases for trace/logs/metrics exemplar:

  • "traceId" key is not present
  • "traceId" key is present and has an empty value
  • "traceId" key is present and value is all 0s

Can we consider these 3 cases the same? Is there a difference between them, if yes, what is the semantic?

@jmacd
Copy link
Contributor

jmacd commented Nov 17, 2022

I asked @bogdandrutu because I didn't understand immediately how #2930 (review) was supposed to block stabilizing OTLP/JSON.

There are more cases than the three listed above. We expect that TraceIDs should be exactly 32 hex digits and SpanIDs should be exactly 16 hex digits. This follows from the conditions for a "valid" TraceID and SpanID here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#spancontext

We already have language about rejecting invalid data, which makes me think no more specification is needed. We should reject IDs with all zeros, whether it is because they are empty or missing, because valid identifiers require at least one non-zero byte.

@tigrannajaryan
Copy link
Member Author

We already have language about rejecting invalid data, which makes me think no more specification is needed. We should reject IDs with all zeros, whether it is because they are empty or missing, because valid identifiers require at least one non-zero byte.

This is true for Spans but not for LogRecords, where traceid/spanid are optional fields.

@jmacd
Copy link
Contributor

jmacd commented Nov 17, 2022

This is true for Spans but not for LogRecords, where traceid/spanid are optional fields.

Bogdan's remark was clear about presence and absence, though #2930 (review).

If the "traceId" is absent, then the field is absent, and if the "traceId" is present then the field is present. We only expect valid data when a field is present.

@github-actions
Copy link

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 label Nov 25, 2022
@bogdandrutu
Copy link
Member

If the "traceId" is absent, then the field is absent, and if the "traceId" is present then the field is present. We only expect valid data when a field is present.

Does that mean absent is equivalent with present and invalid?

@Aneurysm9
Copy link
Member

Aneurysm9 commented Nov 29, 2022

Does that mean absent is equivalent with present and invalid?

If the field is required, then I would say yes. If the field is not required, then no.

@github-actions
Copy link

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

@carlosalberto
Copy link
Contributor

Hey @tigrannajaryan

Adding a small note after (briefly) discussing this: the question regarding trace_id being missing/empty/all-zeroes is specially important as we are deviating from the standard protbuf/JSON handling for trace_id, hence having this super clear would be important for components such as the Collector.

As discussed: we should at least mention some of the mentioned cases (missing/empty trace_id) is undefined behavior.

cc @bogdandrutu @jmacd

@abitrolly
Copy link

We need a lot of eyes on this before we merge this.

I can not read the spec. I need to read examples, but there are no examples as long as open-telemetry/opentelemetry-proto#462 is open.

@abitrolly
Copy link

abitrolly commented Dec 20, 2022

Another concern is that spec doesn't cover how to deal with incomplete and missing spans, which should occur quite often in case of errors and exceptions. Basically in cases where the proper trace would be most useful.

#2773 (comment)
#373 (comment)

@tigrannajaryan
Copy link
Member Author

Depends on #3040

@tigrannajaryan
Copy link
Member Author

Would like to clarify the behavior for invalid (all 0) trace/span IDs. In other words we have 3 cases for trace/logs/metrics exemplar:

  • "traceId" key is not present
  • "traceId" key is present and has an empty value
  • "traceId" key is present and value is all 0s

Can we consider these 3 cases the same? Is there a difference between them, if yes, what is the semantic?

Here is a PR to clarify this: open-telemetry/opentelemetry-proto#442

@github-actions
Copy link

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 label Jan 11, 2023
@tigrannajaryan
Copy link
Member Author

Not stale. Blocked by open-telemetry/opentelemetry-proto#442

@carlosalberto
Copy link
Contributor

@tigrannajaryan the last blocking issue has been resolved - I think it's time to merge it (and release it as part of the February release).

@carlosalberto carlosalberto mentioned this pull request Feb 3, 2023
@tigrannajaryan
Copy link
Member Author

This is ready to be merged after 1.18 release.

@abitrolly
Copy link

IWithout making end of span optional you will have to break the stable implementation, or people will resort to hacks for debugging spans in real-time.

@carlosalberto
Copy link
Contributor

@abitrolly Probably a comment for a different PR/issue? Please follow up otherwise with a new issue ;)

@carlosalberto
Copy link
Contributor

Merging as 1.18.0 was released.

@carlosalberto carlosalberto merged commit a1902b6 into open-telemetry:main Feb 9, 2023
@tigrannajaryan tigrannajaryan deleted the feature/tigran/declare-otlp-json-stable branch February 9, 2023 20:02
@jonatan-ivanov
Copy link
Member

Is there any roadblock in front of marking the whole OTLP spec stable (right now it is mixed but its components are stable)?

jonatan-ivanov added a commit to jonatan-ivanov/opentelemetry-specification that referenced this pull request Mar 2, 2023
Since OTLP/JSON was declared stable, all the parts of the OTLP spec is stable
so the whole spec should be declared as such
See open-telemetrygh-2930

Signed-off-by: Jonatan Ivanov <jonatan.ivanov@gmail.com>
jonatan-ivanov added a commit to jonatan-ivanov/opentelemetry-specification that referenced this pull request Mar 2, 2023
Since OTLP/JSON was declared stable, all the parts of the OTLP spec is stable
so the whole spec should be declared as such
See open-telemetrygh-2930

Signed-off-by: Jonatan Ivanov <jonatan.ivanov@gmail.com>
jonatan-ivanov added a commit to jonatan-ivanov/opentelemetry-specification that referenced this pull request Mar 2, 2023
Since OTLP/JSON was declared stable, all the parts of the OTLP spec is stable
so the whole spec should be declared as such
See open-telemetrygh-2930

Signed-off-by: Jonatan Ivanov <jonatan.ivanov@gmail.com>
jonatan-ivanov added a commit to jonatan-ivanov/opentelemetry-specification that referenced this pull request Mar 4, 2023
Since OTLP/JSON was declared stable, all the parts of the OTLP spec is stable
so the whole spec should be declared as such
See open-telemetrygh-2930

Signed-off-by: Jonatan Ivanov <jonatan.ivanov@gmail.com>
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.

None yet