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

fix: include OTel semconv 1.20 attributes in spans #306

Merged
merged 6 commits into from
Oct 30, 2023

Conversation

robbkidd
Copy link
Member

@robbkidd robbkidd commented Oct 27, 2023

Which problem is this PR solving?

The 1.21+ HTTP attributes are not yet stable. Per the HTTP semantic convention v1.21/v1.22 guidance, we should send the "old" style attributes. Doing so will help the agent's telemetry play better with others when appearing alongside application instrumentation in the same set of services.

Short description of the changes

  • Update handler to also include the v1.20 semconv attributes other OTel instrumentation is currently using so that existing queries match.

Proposed future followup:

  • Add the OTEL_SEMCONV_STABILITY_OPT_IN flag for opting in to the new, as-yet unstable attribute names?

The 1.21+ HTTP attributes are not yet stable. Update handler to use the
attributes other current instrumentation is using so that existing
queries match.
@robbkidd robbkidd requested a review from a team October 27, 2023 18:14
@robbkidd
Copy link
Member Author

robbkidd commented Oct 27, 2023

I reckon we can decide on whether/how to implement the OTEL_SEMCONV_STABILITY_OPT_IN flag in a followup!

I'll add some tests to expect these attributes to appear and then wrap up this PR as-is.

This change got more invasive than I initially intended. I extracted
most of the attribute setting to a function that returns a slice of
attributes. This avoids needing to create a span and retrieve it from
some test exporter to assert the which attributes are produced by which
logic branches.

The test is not yet comprehensive of those branches, though.
Wrapped chunks of the test in t.Run() to give them more visible names
when the test suite runs.
@robbkidd
Copy link
Member Author

Might have gotten carried away with refactoring the code to make unit testing simpler. 😬

@robbkidd robbkidd added the type: bug Something isn't working label Oct 27, 2023
@robbkidd robbkidd self-assigned this Oct 27, 2023
@robbkidd robbkidd added this to the Pre beta milestone Oct 27, 2023
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

I like what your refactoring is doing. Maybe it could go one step further so the handleEvent func is responsible for creating the span, then we have handleHTTPEvent funcs that gather the specific event type attrs and the span name. This will make adding more event type handlers easier / cleaner.

Something like:

func handleHTTPEvent(event) (spanName string, attrs []attribute.keyvalye)

@robbkidd
Copy link
Member Author

Maybe it could go one step further …

😈 You read my mind, @MikeGoldsmith, but I held off because I thought I was already going pretty far in this refactor.

I considered again having a handleHTTPEvent() return a span name and slice of attributes, but I think Not Right Now™. There's some http. attrs we set in the wibblyWobblyTimeyWimey getEventStartEndTimestamps() function. Maybe further handler logic separate can happen in a followup PR, maybe when we've got a second network protocol to handle.

Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

This is great! I suggested a few more attributes to a test case but then I think this is good to go!

handlers/otel_handler_test.go Show resolved Hide resolved
@JamieDanielson
Copy link
Contributor

Also noting this table for quick reference, in preparation for docs updates:

"http.request.method" / "http.method"
"http.response.status_code" / "http.status_code"
"http.request.body.size" / "http.request_content_length"
"http.response.body.size" / "http.response_content_length"
"url.path" / "http.target"

@robbkidd robbkidd merged commit 20e6565 into main Oct 30, 2023
@robbkidd robbkidd deleted the robb.semconv-1-20-attrs branch October 30, 2023 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants