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

Include the calling plugin ID in a request header on all Elasticsearch API calls #77214

Closed
joshdover opened this issue Sep 10, 2020 · 17 comments
Closed
Labels
Feature:Telemetry Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@joshdover
Copy link
Contributor

In order to assist with filtering Elasticsearch's usage telemetry, it's been requested that Kibana includes the ID of the plugin that initiated an API call in all requests sent to Elasticsearch. This will allow ES to record usage telemetry in a way that allows ES to determine the source of an API call (eg. APM vs Visualize).

The header name is TBD, but some potential options:

  • x-origin-application
  • x-origin-plugin

Ideally, this is implemented in Core and automatically configured in the ES client instances passed to plugins.

@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Telemetry Team:KibanaTelemetry labels Sep 10, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry)

@joshdover
Copy link
Contributor Author

@polyfractal Do you have an input on the header name here? This is something we should be able to easily add whenever Elasticsearch is ready for it.

@polyfractal
Copy link
Contributor

Thanks for opening this @joshdover! Lemme touch base with a few other ES folks to make sure we're all on the same page regarding this change, since it will affect a few different teams. Will be back when I have more info :)

@polyfractal
Copy link
Contributor

Sorry I thought I had posted this comment but apparently never hit the button 🤦

Had a few conversations and the ES side is 👍 with moving forward with this. We're not quite sure when we'll get the implementation done on our side, but getting the headers in whenever Kibana has time would be good (ES will just ignore the headers until we add specific implementation for it).

No strong feelings on header name, although I'm slightly partial to x-origin-application, so that non-Kibana applications (users or otherwise) could also use it if they want.

@pgayvallet
Copy link
Contributor

pgayvallet commented Oct 21, 2020

So x-origin-application would be, like, apm or visualize ? I find it a little misleading, as it is using the 'Kibana' definition of 'application'.

I would personally expect something like that so others, non-kibana callers can leverage the feature:

x-origin-application: kibana
x-origin-plugin: apm

@polyfractal
Copy link
Contributor

That's also 100% cool with us too :) We'll take as much granularity as you'd like to provide... can always simplify on our end if it ends up being too granular, or concatenate them together for display purposes, etc.

@Bamieh
Copy link
Member

Bamieh commented Oct 26, 2020

Note that when we call elasticsearch to query monitoring indices to send to telemetry usage data we append this header:

X-QUERY-SOURCE: 'TELEMETRY'

We've added this header long time ago in case we want to filter / track those es queries for cloud billing.


Also we do pass esClient and savedobject client to the fetch function to all collectors. These collectors are ran whenever usage is requested. each plugin has its own collectors to send its usage from. We can simply add a usage header of some sorts to the fetch clients to track exclusive usage of ES for fetching telemetry.

@pgayvallet
Copy link
Contributor

Now that we're sending the x-elastic-product-origin header for #81536, should we align the header name for the calling plugin? Maybe x-elastic-plugin-origin?

@joshdover
Copy link
Contributor Author

should we align the header name for the calling plugin? Maybe x-elastic-plugin-origin?

+1 on this name. @afharo should we prioritize this for 7.14?

@afharo
Copy link
Member

afharo commented Mar 10, 2021

@afharo should we prioritize this for 7.14?

I'd need to check with the ES devs. AFAIUnderstand from the description, the stats are gathered on ES, and Kibana would retrieve them somehow later on to report it. Is my understanding correct?

@joshdover
Copy link
Contributor Author

I'd need to check with the ES devs. AFAIUnderstand from the description, the stats are gathered on ES, and Kibana would retrieve them somehow later on to report it. Is my understanding correct?

Yes, I don't believe we'd need to change anything in the telemetry code for Kibana for this. The expectation is that ES can use this however they see fit to segment their usage stats that they report back to us.

On the Kibana side, the only change needed would be in the ES client itself inside src/core.

@afharo
Copy link
Member

afharo commented Mar 15, 2021

Makes sense to me. Should we prioritize based on when ES will have the bandwidth to generate stats from it? If they already have the logic in place, then +1 to tackle it asap :)

@LeeDr
Copy link

LeeDr commented Apr 22, 2021

Comment from @VijayDoshi
"I think we'll need more than the feature, the name(?) or some identifier of the visualization, dashboard, watcher, alert, object etc. To just get "Lens" would not be nearly as useful as "Vijay's demo viz". "

I tend to agree that the more granular we can make it the better.

@pgayvallet
Copy link
Contributor

pgayvallet commented Apr 23, 2021

@LeeDr this is a distinct issue, I think you wanted to reply to #97934 instead.

@pgayvallet
Copy link
Contributor

Btw, just as a preliminary investigation:

Currently, all callers (plugins and core) are using the same IClusterClient instance. To support this feature, we will need to modify all public client accessor to pass an additional pluginId meta to the underlying call to the internal API (and also modify the internal API to support this new meta, but this part is technically easier)

There is currently two ways for plugins to retrieve an elasticsearch client:

Via the ES service's start contract

  • ElasticsearchServiceStart.client
  • ElasticsearchServiceStart.createClient

We will need to modify these public accessors to call the internal getter with an additional context property, that should contain at least the internal id of the plugin. We also may need to change

e.g

ElasticsearchServiceStart {
   getClient({ caller }: { caller: string }) // would replace the static `client` property
}

// createPluginStartContext
elasticsearch: {
   client: deps.elasticsearch.getClient({ caller: plugin.name }),
   createClient: (type, clientConfig) => deps.elasticsearch.createClient(type, clientConfig, { caller: plugin.name })
}

This would have the side effect to create a dedicated client instance for each individual plugin. As the number of plugin is constant, this is probably ok though

Via core's RequestHandlerContext

  • CoreRouteHandlerContext.elasticsearch.client

Main issues here is that ATM, a RequestHandlerContextContainer has no knowledge of the plugin invoking it:

  private registerCoreContext(coreSetup: InternalCoreSetup) {
    coreSetup.http.registerRouteHandlerContext(
      coreId,
      'core',
      (context, req, res): RequestHandlerContext['core'] => {
        return new CoreRouteHandlerContext(this.coreStart!, req); // we can't get the calling plugin name here atm
      }
    );
  }

To support the feature for client accesses from the RHC, we will need to modify the context to be able to retrieve the plugin name. Note that core's and plugin's RHC shared the same API, and we may want to only allow core to retrieve the information of the invoking plugin, which may even require further changes in the RHC API (but the pragmatic option may be to expose the info anyhow).

Note that we will then be forced to create a child client for every request accessing the ES client, but as RHC.core.elasticsearch.client is lazy loaded, this is probably okay.

@pgayvallet
Copy link
Contributor

I'll consider this as superseded by #97934 and close it. Please feel free to reopen if I'm wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Telemetry Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

7 participants