Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Part2 Server - duplicate support #5400
base: main
Are you sure you want to change the base?
HTTP Semconv migration Part2 Server - duplicate support #5400
Changes from 2 commits
364713b
16b4346
f33c287
d10c65d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to allocate more space than needed when not all attributes are included. Can we have an issue track setting this to the exact size from the start?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it will, this is because optimizing for allocated space is over optimizing. When you measure at the
http.Handler
the extra capacity is reused. I have built a benchmark to demonstrate this, but they exist outside of this code so I didn't include in the PRInstead I've optimized for time spent in the function, by changing the workflow from
Check if Attributes Exist to count
->Create the Slice
->Check if Attributes exist to Append
toMake a Slice
->Check if Attributes exists to Append
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
and append?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p < 0
doesn't invalidate the host. Shouldn't this check the host value as well first?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a copy of the logic from the current implementation.
If there is no server, then it will take what it can from the
request.Host
. If there is a server (server !="") it will use the host from that, and if the port is present it will use that.Notice that last
splitHostPort
only will set port.Check warning on line 62 in instrumentation/net/http/otelhttp/internal/semconv/dup.go
Codecov / codecov/patch
instrumentation/net/http/otelhttp/internal/semconv/dup.go#L60-L62
Check warning on line 112 in instrumentation/net/http/otelhttp/internal/semconv/dup.go
Codecov / codecov/patch
instrumentation/net/http/otelhttp/internal/semconv/dup.go#L110-L112
Check warning on line 127 in instrumentation/net/http/otelhttp/internal/semconv/dup.go
Codecov / codecov/patch
instrumentation/net/http/otelhttp/internal/semconv/dup.go#L125-L127
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The index state is not tracked in a variable. This method is going to be very inflexible to changes. Can we track the index with a variable instead?
Or just use
attrs[:0]
and append to it? That waylen(attrs)
can always be returned?Check warning on line 149 in instrumentation/net/http/otelhttp/internal/semconv/dup.go
Codecov / codecov/patch
instrumentation/net/http/otelhttp/internal/semconv/dup.go#L147-L149
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here regarding state tracking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to rely on the compiler strategy to allocate space for the attributes. Likely over-allocating. Can we similar to above pre-allocate capacity? Or can we change above to also follow this?
Check warning on line 194 in instrumentation/net/http/otelhttp/internal/semconv/dup.go
Codecov / codecov/patch
instrumentation/net/http/otelhttp/internal/semconv/dup.go#L193-L194