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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5400 +/- ##
=======================================
+ Coverage 62.3% 62.5% +0.1%
=======================================
Files 189 190 +1
Lines 11575 11692 +117
=======================================
+ Hits 7221 7312 +91
- Misses 4145 4164 +19
- Partials 209 216 +7
|
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.
Code changes LGTM. This could use a changelog entry, and it looks like there are a few cases that need test coverage
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.
Semantic convention changes look good to me. There are testing coverage gaps that need to be addressed though.
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. |
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.
// Copyright The OpenTelemetry Authors | |
// | |
// Licensed under the Apache License, Version 2.0 (the "License"); | |
// you may not use this file except in compliance with the License. | |
// You may obtain a copy of the License at | |
// | |
// http://www.apache.org/licenses/LICENSE-2.0 | |
// | |
// Unless required by applicable law or agreed to in writing, software | |
// distributed under the License is distributed on an "AS IS" BASIS, | |
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
// See the License for the specific language governing permissions and | |
// limitations under the License. | |
// Copyright The OpenTelemetry Authors | |
// SPDX-License-Identifier: Apache-2.0 |
// old http.target http.scheme net.host.name net.host.port http.scheme net.host.name net.host.port http.method net.sock.peer.addr net.sock.peer.port user_agent.original http.method http.status_code net.protocol.version | ||
// new http.request.header server.address server.port network.local.address network.local.port client.address client.port url.path url.query url.scheme user_agent.original server.address server.port url.scheme http.request.method http.response.status_code error.type network.protocol.name network.protocol.version http.request.method_original http.response.header http.request.method network.peer.address network.peer.port network.transport http.request.method http.response.status_code error.type network.protocol.name network.protocol.version | ||
|
||
const MaxAttributes = 24 |
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.
const MaxAttributes = 24 | |
const maxAttributes = 24 |
// new http.request.header server.address server.port network.local.address network.local.port client.address client.port url.path url.query url.scheme user_agent.original server.address server.port url.scheme http.request.method http.response.status_code error.type network.protocol.name network.protocol.version http.request.method_original http.response.header http.request.method network.peer.address network.peer.port network.transport http.request.method http.response.status_code error.type network.protocol.name network.protocol.version | ||
|
||
const MaxAttributes = 24 | ||
attrs := make([]attribute.KeyValue, MaxAttributes) |
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 PR
Instead 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
to Make a Slice
-> Check if Attributes exists to Append
.
} else { | ||
// Prioritize the primary server name. | ||
host, p = splitHostPort(server) | ||
if p < 0 { |
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?
if p < 0 { | |
if host == "" && p < 0 { |
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.
attrs[1] = semconvNew.HTTPRequestMethodGet | ||
} | ||
attrs[2] = semconvNew.HTTPRequestMethodOriginal(method) | ||
return 3 |
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 way len(attrs)
can always be returned?
} | ||
attrs[0] = semconvOld.HTTPSchemeHTTP | ||
attrs[1] = semconvNew.URLScheme("http") | ||
return 2 |
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.
// new http.request.header server.address server.port network.local.address network.local.port client.address client.port url.path url.query url.scheme user_agent.original server.address server.port url.scheme http.request.method http.response.status_code error.type network.protocol.name network.protocol.version http.request.method_original http.response.header http.request.method network.peer.address network.peer.port network.transport http.request.method http.response.status_code error.type network.protocol.name network.protocol.version | ||
|
||
const MaxAttributes = 24 | ||
attrs := make([]attribute.KeyValue, MaxAttributes) |
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
attrs := make([]attribute.KeyValue, MaxAttributes) | |
attrs := make([]attribute.KeyValue, 0, MaxAttributes) |
and append?
// | ||
// If any of the fields in the ResponseTelemetry are not set the attribute will be omitted. | ||
func (d dupHTTPServer) ResponseTraceAttrs(resp ResponseTelemetry) []attribute.KeyValue { | ||
attributes := []attribute.KeyValue{} |
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?
This change adds the duplicate attribute producer to the semconv of otlehttp.
The full PR is #5092
Part of #5331