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 Part3 Server - v1.24.0 support #5401

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MadVikingGod
Copy link
Contributor

This change adds the new semantic version (v1.24.0) attribute producer to the semconv of otlehttp.

The full PR is #5092
Part of #5331

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 78.78788% with 21 lines in your changes missing coverage. Please review.

Project coverage is 62.5%. Comparing base (30ed923) to head (5049d91).
Report is 218 commits behind head on main.

Current head 5049d91 differs from pull request most recent head 6db311c

Please upload reports for the commit 6db311c to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5401     +/-   ##
=======================================
+ Coverage   62.3%   62.5%   +0.1%     
=======================================
  Files        189     190      +1     
  Lines      11575   11673     +98     
=======================================
+ Hits        7221    7296     +75     
- Misses      4145    4161     +16     
- Partials     209     216      +7     
Files Coverage Δ
...entation/net/http/otelhttp/internal/semconv/env.go 84.6% <100.0%> (+9.6%) ⬆️
...ntation/net/http/otelhttp/internal/semconv/util.go 83.3% <62.5%> (-16.7%) ⬇️
...tion/net/http/otelhttp/internal/semconv/v1.24.0.go 80.5% <80.5%> (ø)

... and 1 file with indirect coverage changes

@MadVikingGod MadVikingGod added the Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG label Apr 18, 2024
@MadVikingGod
Copy link
Contributor Author

Changelog will be part of follow on PR as part of #5331

Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

Could use a few more test cases, but LGTM otherwise

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

There is missing test coverage.

} else {
// Prioritize the primary server name.
host, p = splitHostPort(server)
if p < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if p < 0 {
if host == "" && p < 0 {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal of this if branch is only to change the port if one wasn't part of server. This was logic from semconvutil:

var host string
var p int
if server == "" {
host, p = splitHostPort(req.Host)
} else {
// Prioritize the primary server name.
host, p = splitHostPort(server)
if p < 0 {
_, p = splitHostPort(req.Host)
}
}

resp: ResponseTelemetry{
StatusCode: 200,
ReadBytes: 701,
ReadError: fmt.Errorf("read error"),

Choose a reason for hiding this comment

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

Suggested change
ReadError: fmt.Errorf("read error"),
ReadError: errors.New("read error"),

@MrAlias MrAlias added this to the v1.28.0 milestone May 30, 2024
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants