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

HTTP Semconv migration Part1 Server - v1.20.0 support #5333

Merged
merged 26 commits into from
Apr 16, 2024

Conversation

MadVikingGod
Copy link
Contributor

This change adds an absctraction layer to the semconv of otlehttp. This is to enable compatibility mode that was required in the v1.21.0 semantic convention.

The full PR is #5092
Part of #5331

@MadVikingGod MadVikingGod requested a review from a team March 29, 2024 13:35
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 96.55172% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 62.3%. Comparing base (0ebeecf) to head (324cb8d).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5333     +/-   ##
=======================================
+ Coverage   62.1%   62.3%   +0.1%     
=======================================
  Files        186     189      +3     
  Lines      11537   11575     +38     
=======================================
+ Hits        7176    7219     +43     
+ Misses      4149    4146      -3     
+ Partials     212     210      -2     
Files Coverage Δ
instrumentation/net/http/otelhttp/handler.go 91.6% <100.0%> (+4.4%) ⬆️
...ntation/net/http/otelhttp/internal/semconv/util.go 100.0% <100.0%> (ø)
...tion/net/http/otelhttp/internal/semconv/v1.20.0.go 100.0% <100.0%> (ø)
...entation/net/http/otelhttp/internal/semconv/env.go 75.0% <75.0%> (ø)

@MrAlias
Copy link
Contributor

MrAlias commented Mar 29, 2024

Please address the lint failures.

@MadVikingGod MadVikingGod added the Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG label Mar 29, 2024
Removed benchmark
Updated license header to SPDX format
Restored splitHostPort Test
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.

I haven't reviewed the code. I just focused on resolving some of the addressed comments.

Signed-off-by: Aaron Clawson <MadVikingGod@users.noreply.github.com>
@pellared
Copy link
Member

pellared commented Apr 8, 2024

Side note, please do not wait on my review.

@MadVikingGod MadVikingGod merged commit 30ed923 into open-telemetry:main Apr 16, 2024
23 checks passed
@MadVikingGod MadVikingGod deleted the mvg/semconv/server/old branch April 16, 2024 21:06
scorpionknifes pushed a commit to scorpionknifes/opentelemetry-go-contrib that referenced this pull request Apr 24, 2024
…#5333)

* added interface around semconvutil
---------

Signed-off-by: Aaron Clawson <MadVikingGod@users.noreply.github.com>
scorpionknifes pushed a commit to scorpionknifes/opentelemetry-go-contrib that referenced this pull request Apr 24, 2024
…#5333)

* added interface around semconvutil
---------

Signed-off-by: Aaron Clawson <MadVikingGod@users.noreply.github.com>
@MrAlias MrAlias added this to the v1.26.0 milestone Apr 24, 2024
// ServerStatus returns a span status code and message for an HTTP status code
// value returned by a server. Status codes in the 400-499 range are not
// returned as errors.
func ServerStatus(code int) (codes.Code, string) {
Copy link

@krim krim Aug 30, 2024

Choose a reason for hiding this comment

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

@MadVikingGod Could you explain why we cannot use codes.Ok for statuses in the range of 200..399, or at least for 2xx?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG
Projects
Development

Successfully merging this pull request may close these issues.

6 participants