Skip to content

Conversation

@gregkalapos
Copy link
Contributor

@gregkalapos gregkalapos commented Feb 28, 2025

@AlexanderWert pointed out that some services in the OTel demo send GRPC spans with very few SemConv attributes. Here is an example of such a span from the recommendation python service:

{
    "attributes": {  
      "rpc.grpc.status_code": 0,
      "rpc.method": "ListProducts",
      "rpc.service": "oteldemo.ProductCatalogService",
      "rpc.system": "grpc",
    },

Although according to SemConv RPC spans have required attribute server.address - it's not set by this python service. None of the other fields we use for destination.service.resource are set, but we have rpc.service.

The problem with this is that without destination.service.resource, the service map won't show this dependency.

@AlexanderWert suggested to then just use this field as a fallback and set that as destination.service.resource, which I think makes sense, because this field is needed for service map and rpc.service still identifies the target service.

Service map prior to this PR:
Screenshot 2025-02-28 at 18 55 05

Service map with this PR:
Screenshot 2025-02-28 at 18 39 34

Note that the real service is still not resolved on the map; I'm not sure it's related to this thing... I see that all the time, also when the call happens via HTTP. So far my guess is that it's not relevant, but if someone knows more, let me know.

@gregkalapos gregkalapos self-assigned this Feb 28, 2025
@gregkalapos gregkalapos requested a review from a team as a code owner February 28, 2025 17:52
@gregkalapos
Copy link
Contributor Author

gregkalapos commented Mar 3, 2025

@AlexanderWert brought up if we should also add this in apm-data.

My thoughts on this: the fix itself is very small, from what I see, we can just add it here. So that's not a challenge, I'm happy to open a PR against apm-data as well.

On the other hand, I think it'd be nice to have some framework on what we backport to apm-data and what we don't. This one is very visible, because it's in the OTel demo, but I'd say it's more of an edge case where the OTel SDK does not comply with semconv, so we go beyond what'd be standard OTel support. server.address is required, which is not sent, and the other network.peer.* fields are optional, but not sent either. So what we do here is like a best effort fix-up of the non-compliant data. Doing it in the collector makes sense to me, but I'm unsure how much of this we want to backport to apm-data. Happy to hear opinions.

Update: discussed via slack: we won't port this to apm-data.

Copy link
Contributor

@lahsivjar lahsivjar left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +466 to +468
} else {
// fallback to RPC service
destnResource = s.rpcService
Copy link
Contributor

Choose a reason for hiding this comment

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

We already use rpc.service for setting service.target.name, so , it makes sense to me to use it for setting destination resource too.

@gregkalapos gregkalapos merged commit 4400949 into elastic:main Mar 3, 2025
3 checks passed
@gregkalapos gregkalapos deleted the rpc_destination_service_res_fallback branch March 3, 2025 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants