Skip to content
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

Public API for destination fields #424

Closed
AlexanderWert opened this issue Mar 15, 2021 · 12 comments
Closed

Public API for destination fields #424

AlexanderWert opened this issue Mar 15, 2021 · 12 comments

Comments

@AlexanderWert
Copy link
Member

AlexanderWert commented Mar 15, 2021

Extend the public APIs of the agents to allow setting destination fields.

  • When no destination fields are set explicitly default values should be derived from other fields.
  • Provide detailed API that allows users to explicitly specify destination fields

Relevant fields are:

  • context.destination.address
  • context.destination.port
  • context.destination.service.name
  • context.destination.service.type
  • context.destination.service.resource
  • context.destination.cloud.region

Status

Summary: custom ep custom ep custom ep
Agent Public API extended by destination fields
Java issue details issue details
dot-net issue details issue details
Go issue details issue details
PHP issue details issue details
Ruby issue details issue details
NodeJS issue details issue details
Python issue details issue details
@SergeyKleyman
Copy link
Contributor

Please note that intake API in 7.11 doesn't allow leaving out or setting to null any of context.destination.service.* properties. So it might worth designing API in a way that will require user to provide values for all context.destination.service.* properties.

@felixbarny
Copy link
Member

Another thing to consider is what to do if the users explicitly provide null as an argument. I'd suggest not to throw an exception but rather to log a warning and set all context.destination.service.* fields to null. To avoid excessive logging, the agents should only log once or at least only once every x (ex. 5) minutes.

@eyalkoren
Copy link
Contributor

So it might worth designing API in a way that will require user to provide values for all context.destination.service.* properties.

@SergeyKleyman Makes sense, you mean something like setDestinationService(String type, String name, String resource)? In the same spirit we can add setDestinationAddress(String host, int port).
It's not critical to align, but probably better at least conceptually. We can use the related spec for that.

I'd suggest not to throw an exception but rather to log a warning and set all context.destination.service.* fields to null

@felixbarny you mean - avoid sending or sending null for the context.destination.service field, which is allowed by the intake API?

@felixbarny
Copy link
Member

I mean if someone sets setDestinationService(type = "db", name = "oracle", resource = null), we should not send any context.destination.service.* fields as APM Server would reject that due to it's validation rules.

@SergeyKleyman
Copy link
Contributor

SergeyKleyman commented Mar 16, 2021

you mean something like setDestinationService(String type, String name, String resource)

Yes, something like that. In PHP it's span.destination().setService(String name, String resource, String type) - I used the same order as the one in 7.11 intake API schema.

Another thing to consider is what to do if the users explicitly provide null as an argument.

It's a good point. Of course for languages that support nullability as part of the type system (for example Kotlin, C# 8+ and PHP 7.1+ :) it's less of an issue.

@felixbarny
Copy link
Member

While writing the destination spec (#435), I noticed the fields destination.service.name and destination.service.type are not used in the Server and the UI. There are currently no plans to use them in the future.

Hence, I don't think it makes sense to let users (and ourselves) think hard about which values to set when it doesn't matter at all.
The most relevant destination field is destination.service.resource. In most cases, it can be derived from other span fields, such as the URL, the queue name, and the subtype.

Some options

  1. Remove the fields name and type. As the fields are required in the Intake API, send empty strings to APM Server.
  2. Make name and type internal and always infer them based on other properties.
  3. Expose name and type via the API but infer unset fields. This makes setting all fields optional.

Before implementing changes in the API, we should come to a conclusion first.
I'm currently drafting an extension to #435 that specifies how and when to infer destination fields.

@felixbarny
Copy link
Member

felixbarny commented Apr 26, 2021

Update: first early draft: #436

@axw
Copy link
Member

axw commented Apr 27, 2021

FWIW, the Go agent does not currently expose "type" as something that can be set by users; it is always internally set to the span type.

I wonder if we should always infer name, resource, and type in the server? We already have to do that for Jaeger and OpenTelemetry, so why not our own spans? I'm inclined this way, but I'm not sure if we have any use-cases for setting custom destination service context.

In the past I believe the team discussed using destination.service.type for identifying different types of destination services, and using that for choosing a different icon (for example). Is this something we've actively decided not to do, or just something we don't yet have designs for?

@felixbarny
Copy link
Member

I wonder if we should always infer name, resource, and type in the server?

In order to be able to summarize stats for dropped spans (part of the chatty apps proposal #432), agents need to know the resource. But we could definitely infer the name and type in the server.

As the fields are required in the intake API, agents would need to continue to populate the field even if we change the server to do the inference. That's because we want agents to be compatible with new and old APM Server versions. Agents could check the version of the Server and not send the fields for newer versions, though.
But if the fields are not used and we don't have plans to use them in the future, maybe we should just get rid of them? Agents could send empty strings and newer APM Server versions would drop the fields.

In the past I believe the team discussed using destination.service.type for identifying different types of destination services, and using that for choosing a different icon (for example). Is this something we've actively decided not to do, or just something we don't yet have designs for?

@ogupte said

it’s possible we might use the other fields in future enhancements such as context.destination.service.type, but nothing planned at this time.

I think the UI is using span.type to determine the icon on the service map. I have asked the team for confirmation.

@axw
Copy link
Member

axw commented Apr 27, 2021

In order to be able to summarize stats for dropped spans (part of the chatty apps proposal #432), agents need to know the resource. But we could definitely infer the name and type in the server.

Ah right.

As the fields are required in the intake API, agents would need to continue to populate the field even if we change the server to do the inference.

Yep. I was thinking if we could make them all inferred in the server, then the agents could simply omit destination.service altogether, which is permitted. But as you pointed out, we need to send these properties from the agent for dropped span - so not so easy.

@felixbarny
Copy link
Member

I think the UI is using span.type to determine the icon on the service map. I have asked the team for confirmation.

Fields relevant for the icons are: agent.name for instrumented services and span.type and span.subtype for external calls.
https://github.com/elastic/kibana/blob/00589868e2b53072dabd5a9d2d9f0f3da5b8d373/x-pack/plugins/apm/public/components/app/ServiceMap/icons.ts

I've created a proposal about deprecating destination.service.type and destination.service.name and adding an API to set the destination.service.resource, as well as an API to create an exit span: #436

@AlexanderWert
Copy link
Member Author

Replaced by #448

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants