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

[Logs] Add semantic conventions for writing exceptions #2819

Merged

Conversation

CodeBlanch
Copy link
Member

Changes

Defines how exceptions should be written when using the logs & events API.

/cc @jack-berg @scheler @MSNev

@CodeBlanch CodeBlanch requested review from a team as code owners September 21, 2022 20:53
@reyang reyang added spec:logs Related to the specification/logs directory area:semantic-conventions Related to semantic conventions labels Sep 21, 2022
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 with a changelog entry.

Copy link
Contributor

@scheler scheler left a comment

Choose a reason for hiding this comment

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

Interesting addition to Logs semantic conventions. Requires clarification on a few things.

specification/logs/semantic_conventions/exceptions.md Outdated Show resolved Hide resolved
specification/logs/semantic_conventions/exceptions.md Outdated Show resolved Hide resolved
@yurishkuro
Copy link
Member

Could you please explain why this change is needed, specifically why it needs to redefine how exceptions are logged instead of referring to the existing definition under /trace/, as this PR already does for .stacktrace?

@CodeBlanch
Copy link
Member Author

@yurishkuro

Could you please explain why this change is needed, specifically why it needs to redefine how exceptions are logged instead of referring to the existing definition under /trace/, as this PR already does for .stacktrace?

What I brought up on the Log SIG was that logging exceptions is common and the spec should define what to do. We could say "see trace exceptions" (which we kind of do) but the trace document is referencing spans & span events which are not applicable to logs. So the goal here is to prescribe what to do without ambiguity.

What perhaps might be better is to move exceptions.yaml up a level and then have traces + logs reference that?

@yurishkuro
Copy link
Member

Yes, I would support making exceptions top-level category (cf. https://bit.do/telemetry-temple) and referring to it from both traces and logs.

@CodeBlanch CodeBlanch requested a review from a team as a code owner September 26, 2022 23:14
@CodeBlanch
Copy link
Member Author

@yurishkuro I moved exceptions.yaml up a level and it is now referenced by traces & logs. LMK what you think about that.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

I am confused about what this PR does. It seems like we have 3 different yaml files which define almost the same conventions (but not exactly the same).

Do we need the separate yaml files under traces and logs directories?

@CodeBlanch
Copy link
Member Author

@tigrannajaryan

Do we need the separate yaml files under traces and logs directories?

It depends I suppose on where you stand on the DRY principle 😄 The yaml files are set up so that the Semantic Convention generator can spit out the markdown tables from a single source of truth. There a couple subtle (maybe not so) differences between traces & logs wrt to exceptions. Traces use an event off of span and have the escaped attribute.

If we want to jettison the generator and just mirror content manually, that is fine by me. The risk is they might become out of sync for the shared stuff.

@tigrannajaryan
Copy link
Member

@tigrannajaryan

Do we need the separate yaml files under traces and logs directories?

It depends I suppose on where you stand on the DRY principle 😄 The yaml files are set up so that the Semantic Convention generator can spit out the markdown tables from a single source of truth. There a couple subtle (maybe not so) differences between traces & logs wrt to exceptions. Traces use an event off of span and have the escaped attribute.

If we want to jettison the generator and just mirror content manually, that is fine by me. The risk is they might become out of sync for the shared stuff.

I guess I am confused because I don't know how the generator works. Are you saying that the yaml files in traces and logs subdirectories reference the common yaml file?

@CodeBlanch
Copy link
Member Author

@tigrannajaryan

I guess I am confused because I don't know how the generator works. Are you saying that the yaml files in traces and logs subdirectories reference the common yaml file?

💯! In the subdirectories you'll see ref used in the yamls like these in the log version. Then in the markdown you use the semconv tag thing like this in the log version which tells the generator to build the table in that location. The table gets built following the refs to the shared yaml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:logs Related to the specification/logs directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants