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

Update http Semconv to v1.20.0 #4320

Merged
merged 11 commits into from Dec 12, 2023

Conversation

MadVikingGod
Copy link
Contributor

This updates all http instrumentation (anything using internal/shared/semconvutil) to v1.20.0.

In addition, I did an audit of the currently recommended semantic conventions for HTTP and put a comment of what is included and what is not to hopefully be easier to reference on the next update.

This doesn't go to 1.21 as that included a new set of logic around the OTEL_SEMCONV_STABILITY_OPT_IN environment variable. That will require a release using 1.20.0 for this to work correctly.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #4320 (59ecc7a) into main (f7ca3ec) will increase coverage by 0.2%.
The diff coverage is 94.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4320     +/-   ##
=======================================
+ Coverage   80.8%   81.1%   +0.2%     
=======================================
  Files        150     150             
  Lines      10374   10759    +385     
=======================================
+ Hits        8388    8731    +343     
- Misses      1844    1872     +28     
- Partials     142     156     +14     
Files Coverage Δ
...estful/otelrestful/internal/semconvutil/netconv.go 100.0% <100.0%> (ø)
...hub.com/emicklei/go-restful/otelrestful/restful.go 100.0% <ø> (ø)
...ation/github.com/gin-gonic/gin/otelgin/gintrace.go 83.1% <ø> (ø)
...-gonic/gin/otelgin/internal/semconvutil/netconv.go 100.0% <100.0%> (ø)
...orilla/mux/otelmux/internal/semconvutil/netconv.go 100.0% <100.0%> (ø)
...trumentation/github.com/gorilla/mux/otelmux/mux.go 89.2% <ø> (ø)
...entation/github.com/labstack/echo/otelecho/echo.go 100.0% <ø> (ø)
...tack/echo/otelecho/internal/semconvutil/netconv.go 100.0% <100.0%> (ø)
...ron.v1/otelmacaron/internal/semconvutil/netconv.go 100.0% <100.0%> (ø)
...ntation/gopkg.in/macaron.v1/otelmacaron/macaron.go 91.4% <ø> (ø)
... and 12 more

@dmathieu
Copy link
Member

Nice! :)
How about a changelog entry?

@MadVikingGod MadVikingGod added this to the v0.45.0 milestone Sep 26, 2023
@MrAlias MrAlias modified the milestones: v1.20.0/v0.45.0, v0.46.0 Sep 28, 2023
@MrAlias
Copy link
Contributor

MrAlias commented Oct 26, 2023

From SIG meeting:

  • @MadVikingGod ran a semconv checker against this and found things that need to be changed or documented. He plans to update with that info/changes.

@MadVikingGod
Copy link
Contributor Author

MadVikingGod commented Oct 26, 2023

So they aren't lost, this was the results of running the example from the sig meeting:

2023/10/26 12:54:07 INFO missing attributes type=trace section=span scope.name=go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp name=http.server.root attributes="[http.client_ip net.sock.host.addr net.sock.host.port http.route]"
2023/10/26 12:54:07 INFO extra attributes type=trace section=span scope.name=go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp name=http.server.root attributes="[net.sock.peer.port user_agent.original net.protocol.version http.method net.sock.peer.addr http.wrote_bytes http.status_code]"
2023/10/26 12:54:07 INFO missing attributes type=trace section=span scope.name=go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp name=http.client.GET. attributes=[http.resend_count]
2023/10/26 12:54:07 INFO extra attributes type=trace section=span scope.name=go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp name=http.client.GET. attributes="[http.status_code http.response_content_length http.method]"

From this The server is missing:

  • http.client_ip - Recommended
  • net.sock.host.addr - Opt-In
  • net.sock.host.port - Conditionally Required: If defined for the address family and if different than net.host.port and if net.sock.host.addr is set. In other cases, it is still recommended to set this.
  • http.route - Conditionally Required: If and only if it's available

And the Client is missing:

  • http.resend_count - Recommended: if and only if request was retried.

Note these may be optional or not accessible.

@MadVikingGod
Copy link
Contributor Author

I think we are compliant:

  • http.client_ip - Is recorded from X-Forwarded-For, but we didn't have a proxy so it is absent in the test.
  • net.sock.host.addr - It is Opt-in, we don't have the config option for it, and I don't know if we can access this data from within the handler.
  • net.sock.host.port - Requires net.sock.host.addr, and isn't included for the same reasons.
  • http.route - This is stored at the router/mux level and not accessible to the handler.
  • http.resend_count - Resends must happen in the application code above the instrumented client. A client request will only ever send once.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

@pellared
Copy link
Member

pellared commented Dec 6, 2023

@MadVikingGod Any update on this PR?

@pellared pellared modified the milestones: v0.46.0, v0.48.0 Dec 12, 2023
@pellared pellared added this to the v0.47.0 milestone Dec 12, 2023
@MrAlias MrAlias merged commit 74f4b7f into open-telemetry:main Dec 12, 2023
22 checks passed
pellared added a commit to pellared/opentelemetry-go-contrib that referenced this pull request Dec 12, 2023
MrAlias pushed a commit that referenced this pull request Dec 12, 2023
@MadVikingGod MadVikingGod deleted the mvg/update-semconv branch December 13, 2023 16:18
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

4 participants