Make server.socket.* attributes on the HTTP server side opt-in
#8747
Conversation
| @CanIgnoreReturnValue | ||
| public HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> setCaptureServerSocketAttributes( | ||
| boolean captureServerSocketAttributes) { | ||
| this.captureServerSocketAttributes = captureServerSocketAttributes; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
My understanding is that we don't have to support opt-in things. I'd personally suggest we not provide this configuration unless someone has specific need now, and we can wait to see how your advice PR pans out.
There was a problem hiding this comment.
Instrumentations SHOULD populate the attribute if and only if the user configures the instrumentation to do so. Instrumentation that doesn't support configuration MUST NOT populate Opt-In attributes.
It's not very explicit about optional support for these; although there is a "SHOULD" here, so it flies, I think. I'm fine with that; and also this is a good subject for the general advice proposed in #3557
mateuszrzeszutek
force-pushed
the
optional-server-socket-attributes
branch
from
f3ae319
to
1043077
Compare
mateuszrzeszutek
force-pushed
the
optional-server-socket-attributes
branch
from
1043077
to
655ad34
Compare
|
@open-telemetry/java-instrumentation-approvers please review this PR 🙏 |
|
@lmolkova do you remember why |
mateuszrzeszutek
force-pushed
the
optional-server-socket-attributes
branch
from
655ad34
to
b45ffd7
Compare
One reason I can think about is that they represent local host/port and are the same for all spans coming from a typical single-host application - this way they're captured best with resource attributes |
This is just the span attributes; for metric attributes I'm hoping that my idea to use the advice API catches on.