added: peer service attribute to Guzzle#581
Conversation
|
@paulgrav - we are actively working on fixing CI - once your draft is ready for review, I'll review it and merge. |
| ->spanBuilder(sprintf('%s', $request->getMethod())) | ||
| ->setParent($parentContext) | ||
| ->setSpanKind(SpanKind::KIND_CLIENT) | ||
| ->setAttribute(TraceAttributes::PEER_SERVICE, $request->getUri()->getHost()) |
There was a problem hiding this comment.
The schematic convention says:
The
service.nameof the remote service. SHOULD be equal to the actualservice.nameresource attribute of the remote service if any.
Are we sure that the $request->getUri()->getHost() will return the service.name here?
There was a problem hiding this comment.
In that case then perhaps neither HttpClientInstrumentation nor GuzzleInstrumentation should set PEER_SERVICE?
Only the client of those libraries can know the PEER_SERVICE name, but there’s no good way to set it unless you write your own hooks.
For where I work, the host name would make a good proxy for the service.name.
I’d like to know the thinking behind this: line 65
There was a problem hiding this comment.
@paulgrav, I mean, I was expecting this, but at the same time, I don't think having something configured incorrectly elsewhere is a good justification for continuing the same (incorrect) pattern in new places. A much better approach would be to fix the existing incorrect behavior rather than following it.
There was a problem hiding this comment.
I can’t disagree. Looking at HttpClientInstrumentation, the PEER_SERVICE attribute should probably be SERVICE_ADDRESS.
There was a problem hiding this comment.
Not sure what is SERVICE_ADDRESS, but I can only think of this attribute: https://opentelemetry.io/docs/specs/semconv/registry/attributes/network/#network-peer-address
And, if we ALL think that's the right one, it should be updated everywhere.
There was a problem hiding this comment.
@bobstrecansky / @ChrisLightfootWild, what do you say about the existing instrumentations that use service.name whereas server.address should have been used. Can we just change it?
@paulgrav provided such an example:
https://github.com/paulgrav/opentelemetry-php-contrib/blob/38f145c1e29b406110c94999772def3e0557bf52/src/Instrumentation/Symfony/src/HttpClientInstrumentation.php#L65
There was a problem hiding this comment.
The way I'm reading: https://opentelemetry.io/docs/specs/semconv/general/attributes/
is:
service.name and server.address serve two completely different scopes. service.name identifies the logical application emitting the telemetry, while server.address is a network attribute identifying the specific host or IP of a target system being called
There was a problem hiding this comment.
I'm wondering if the Github history will tell us anything here, or if it was just a mistake.
There was a problem hiding this comment.
Java appears to have peer service mapping functionality:
There was a problem hiding this comment.
I think server.address is the correct way forward. With the lacking functionality to do service mapping ideally being added to in future 🤔
4 participants
The HttpClientInstrumentation sets the PEER_SERVICE attribute whilst GuzzleInstrumentation.php does not.
Grafana’s Traces Drilldown view uses the PEER_SERVICE attribute to make the spans more readable.
This PR aligns behaviour between the two instrumentations.