Simplify heuristic for ES cluster name

SylvainJuge commented 2 years ago

/!\ Update: the current state of the proposal is summarized in this comment below.

Initial idea was to capture Elasticsearch cluster name using this strategy.

(1) use cluster name stored in ES clients if there is such (as of now, no such thing exists)
(2) use value of x-found-handling-cluster HTTP response header which is provided in Cloud
(3) passively capture the cluster_name in one of the client responses.
(4) lazily execute a request to ES to retrieve cluster name through an explicit call to ES

However, after doing a PoC with Java agent (https://github.com/elastic/apm-agent-java/pull/2796) and talking with @swallez (ES clients team): The cluster_name defaults to elasticsearch , but is usually set by the user for on-premises deployments, in cloud it is a random hex string like 3a29c1a34e2fba92d245020741e38977.

While not really "user-friendly" for cloud, it at least provide a stable value we can rely on for cardinality
One instance of an ES client only communicates with a single cluster, it is thus safe to cache it at the client level.
Alternatively we could also rely on the cloud deployment name which is available through explicit queries on ES, for example with GET /_cluster/settings.

For (1), if there was such storage it would be added only in newer clients

It would only work for newer clients, we still have to rely on (2,3,4) for existing clients
Implementation would be similar as (2,3,4), with the difference that it would be included in the client. One downside though is that client does not have any direct use for this and thus guarding against regressions might be tricky over time.
For context, in Java, there is a total of 3 clients, REST "high-level", REST "low-level" (the one our agent instruments), and the "new Java client" which code is now 99% generated and relies on the REST "low-level"
For Java, because we instrument the REST low level client, we would have to propagate the "cluster_name" from an high-level client, which might be tricky to cross 2/3 intermediate layers.

For (2), the implementation is trivial, the only downside is that it's not available outside cloud.

For context, this header is used by the cloud proxy to route the request/response to the right cluster. It seems to be even possible to avoid using the dedicated DNS of the cluster and directly use the cloud proxy with this as a request HTTP header.

For (3), in order to be relevant, it would assume that the application does make a call to GET / or similar, which is very unlikely on most applications.

For java, there are a few technical complications: it requires extra buffering of the response and avoid side effects, it would not work if agent is attached at runtime after the request/response is complete. So complexity with low probability of relevance means it's probably not worth it for Java.

For (4), it seems to be an acceptable compromise in most cases, but there are caveats

using GET / is enough to get the cluster_name
When a request to ES is made, the agent has to "borrow" credentials from the application request to issue its own request.
- For Java, due to having both synchronous/asynchronous variants, it's easier to execute the extra request before the application request for access to HTTP headers, doing so when the response is available implies extra complexity. On top of that, because the response header is not yet available before the request executes it means the agent would always execute the extra request, unless we can detect using cloud and assume the HTTP response header will be available.
- Making an extra request is not without side-effects, we should provide a way to opt-out
  - History: In 7.14 clients, an extra "product check" request GET / was executed once on the first client request, with 8.x this is replaced by a fancy content-type header in response. One issue this approach was with lambda/faas when a single query to ES is made, it would simply double the number of queries. This is of course less an issue with regular backend applications.
  - If we could reliably detect running in an Lambda/faas then disabling automatically this feature could be relevant.

Proposal

A) remove option (1) as it provide little benefit/cost ratio (same impl as other options + not covering existing clients)
B) remove option (3) as it is quite likely not relevant for most applications (telemetry on ES span names could help us here, but I don't see why applications would explicitly perform this type of query on a regular basis)
C) add to option (4) that the extra querying should be avoided in following cases:
- When lambda/faas deployment is used. Can we reliably detect this and how ?
- When using a cloud ES instance, the response header should be used. Can we reliably detect this and how ? (this might be quite specific to Java agent, but I assume that any asynchronous client might have the same issue).

Things to clarify/discuss

[x] can we reliably detect cloud ES deployment at runtime and disable extra query ?
- Answer: NO, we can't guess it from either the request headers or URLs, even if most share a suffix.
[x] can we reliably detect FAAS execution context and disable extra query ?
- Answer: YES, this will likely be done through env. variables and we already populate related fields in faas agent protocol property.
- Update: this is not required anymore after switching to opt-in rather than opt-out, this extra request will not be issued by default in FAAS environments.
[x] do we need to add an option to opt-out of this feature as fallback ?
- Yes, following this feedback switching to opt-in rather than opt-out is preferable.
May the instrumentation collect sensitive information, such as secrets or PII (ex. in headers)?
- [x] n/a
[x] Create PR as draft
[ ] Approval by at least one other agent
[ ] Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
- Remove PM from reviewers if impact on product is negligible
- Remove agents from reviewers if the change is not relevant for them
[ ] Approved by at least 2 agents + PM (if relevant)
[ ] Merge after 7 days passed without objections \ To auto-merge the PR, add /schedule YYYY-MM-DD to the PR description.
[ ] Create implementation issues through the meta issue template (this will automate issue creation for individual agents)

apmmachine commented 2 years ago

:green_heart: Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

#### Build stats * Start Time: 2022-10-24T04:01:00.926+0000 * Duration: 3 min 23 sec

basepi commented 2 years ago

I am pretty allergic to (4). It feels very against our "don't interfere" policy to actually do something to the app. I think I would want it to be opt-in, not opt-out. With the company-wide emphasis on Cloud (which is already solved by (2)), I think opt-in is safer (and most users won't need it since they'll be on Cloud).

I think we risk compromising user trust if we're making unexpected requests against their infra.

SylvainJuge commented 2 years ago

I am pretty allergic to (4). It feels very against our "don't interfere" policy to actually do something to the app. I think I would want it to be opt-in, not opt-out. With the company-wide emphasis on Cloud (which is already solved by (2)), I think opt-in is safer (and most users won't need it since they'll be on Cloud).

I think we risk compromising user trust if we're making unexpected requests against their infra.

That's definitely a good point for making it opt-in rather than opt-out. In this case we could add that agents MAY implement this feature. Also, I don't know if we have telemetry to back this hunch but I don't think that there are a lot of applications communicating with more than one cluster, which means in most cases the extra granularity would not provide extra value for the majority of our users, thus skipping it by default would not be very problematic.

swallez commented 2 years ago

A few additional precisions regarding Cloud deployments (I was in the Cloud team for 4 years before joining Clients):

the cluster_name in GET / is the cluster id, with the following characteristics:
- it is unique across all of Elastic Cloud
- it is immutable (cannot be changed)
- this is the value returned in x-found-handling-cluster
- it is used in Cloud Console URLs: https://cloud.elastic.co/deployments/<cluster-id>
- it is used in cluster URLs, even if a vanity URL was configured for the cluster.
the display_name in GET /_cluster/settings on the other hand:
- can be changed at any time by the user
- is not guaranteed to be unique, even within a user's account
- can be different from the "custom endpoint name" used to build vanity URLs (see screenshot below)
- the display name therefore cannot be used to build the cluster URL, while the cluster id can

For all these reasons, IMHO it would be better to use the cluster id (i.e. cluster_name in GET /) as this is the most stable and most widely used information, with the additional benefit that it can be used to create direct links to the Cloud Console from the APM UI.

My 2 cents 😉

SylvainJuge commented 2 years ago

Proposal have been updated to "disabled + opt-in" for the extra request to ES which simplifies it to two cases:

capture the cluster name from HTTP response header if available
optionally execute an extra request to ES when elasticsearch_fetch_cluster_name option is set to true (defaults to false).

@basepi can you approve this if it works for you so we can move this forward ?

felixbarny commented 2 years ago

The use case we have is that we want to make the service map more useful by showing which cluster(s) an application is talking to.

Right now, we just show something like

graph TD;
  MyApp --> Elasticsearch;

What we want to show is

graph TD;
  MyApp --> esp[Elasticsearch products];
  MyApp --> esu[Elasticsearch users];

While the cluster ID might be globally unique, it's not really as descriptive as the human-readable cluster name. So I think we'd prefer the human-readable name to make the service map more meaningful.

However, this would mean that we don't have a way to get the cluster name without actually calling the cluster settings endpoint. I get Colton's point that doing an extra call might be considered quite intrusive, especially if that means the application responds to user requests slower. But what if we get the cluster name in the background and make sure we only do it once? We could then either buffer the spans until we have the cluster name or just send the first ones without the cluster name.

basepi commented 2 years ago

But what if we get the cluster name in the background and make sure we only do it once? We could then either buffer the spans until we have the cluster name or just send the first ones without the cluster name.

I'm not arguing that we're going to slow down the target infrastructure. The vast majority of our users would likely never notice the additional call. But our policy has always been to not affect the target app, and this would be a change to that policy.

Maybe it's worthwhile. But I don't like it. Does that mean we shouldn't do it? I don't know.

The kicker for me was that this is solved for cloud instances (via the x-found-handling-cluster header). With our continuing focus on cloud it seems like we can be prudent with an opt-in for on-prem, while still getting a lot better data for cloud customers.

I'm open to being overridden, I just wanted to make my opinion known. :)

felixbarny commented 2 years ago

The kicker for me was that this is solved for cloud instances (via the x-found-handling-cluster header)

What I'm questioning is whether the value from this header is descriptive enough. It returns a unique alphanumeric identifier for the cluster (such as d66e0702fa9b48dfbcd629434c4beb83). Only the cluster settings endpoint would give us a more descriptive display name of the cluster (such as release-oblt (4667fb)). So if we want a descriptive identifier, I see no other way than to do this extra call.

We have precedent for potentially introducing extra calls to the DB in the Java agent: when we get the metadata for SQL database connection, some drivers implement connection.getMetaData() in a way that retrieves the meta data from the DB on demand. Therefore, the Java agent has a cache for this.

felixbarny commented 2 years ago

I just had a good discussion with @Mpdreamz on this. The TL;DR outcome was to use x-found-handling-cluster and to try to get Elasticsearch to include the cluster.name in a response header by default. These would be the only two strategies to determine the cluster name.

Some more details:

About the display_name
- Calling GET /_cluster/settings may be problematic as while the client might have permissions to do searches on specific indices, it may not have access to the cluster settings API.
- The display name in the cluster settings is not something standard but a custom piece of cluster metadata that Elastic cloud sets.
- For non-cloud deployments, this is not expected to be set.
The value of x-found-handling-cluster is the cluster.name which can be configured by users.
- However, it's not really a descriptive name in Elastic cloud because it's a generated unique identifier that can't be changed.
- Even if the service map wouldn't contain a very descriptive name for Elasticsearch cloud clusters, we could make the service map show a link to the cloud console which shows the human-readable deployment name. Maybe we'd even show a slightly different icon for the Elasticsearch cluster in the service map for cloud deployments.
- For on-prem deployments, the cluster.name is usually more descriptive.
- The ideal option to get the cluster name for both cloud and on-prem clusters would be if Elasticsearch returns it in a response header. We wouldn't be able to get the cluster name for old on-prem ES versions. Maybe that's fine for now and we don't need another fallback that's more intrusive.

SylvainJuge commented 2 years ago

In order to make some progress here, I suggest the following next steps:

keep the spec relying on HTTP response header in cloud
add an optional configuration elasticsearch_fetch_cluster_name that allows to opt-in on explicit cluster name retrieval
follow-up with another spec update when we have another header for non-cloud ES clusters in case the header is not the same as in cloud

The opt-in allows to cover existing clusters for the users that need extra granularity. Implementation in agents will be optional and is likely to be added when someone asks for it (with the exception of the Java agent where the PR is already almost ready). Also, making this optional allows to change anything in the agents that already implement the cloud header part.

felixbarny commented 2 years ago

add an optional configuration elasticsearch_fetch_cluster_name that allows to opt-in on explicit cluster name retrieval

I'm not sure if it's worth adding that option, at least for now. It's not really a great user experience and it would be hard for users to discover. At least intuitively, I wouldn't expect that setting to get a lot of usage. It also doesn't always work as the user might be missing ES cluster level permissions. Also, it adds complexity to agents and because it's a rare code path, there's a good chance that it contains undetected edge cases or bugs.

I think I would remove it from the Java agent PR and if it turns out that this is a frequently requested feature, we can think again on how to enable it uniformly across all agents.

SylvainJuge commented 2 years ago

Thanks for your input @felixbarny , I agree with you it's simpler if we don't include this extra option for now, as you said, if needed we can always add it later. I'll update this PR accordingly so we can move forward.

Also, the implementation in the Java agent was quite tricky (and thus potentially brittle) because agents have to reuse the request header credentials and use them after the initial response is available.

When fetching the cluster name (unlike the cloud "display name" which is the user-visible name of the cloud deployment but requires some extra permissions), we just make an extra query to / , which does not require any extra permission as far as I know (this was also the request used for the "product check" in late 7.x clients).

Thus the updated plan is now even simpler now:

use the cluster name provided by x-found-handling-cluster in HTTP response if present.

elastic / apm