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

Support additional context in metrics prefix #1789

Closed
asifsmohammed opened this issue Sep 19, 2022 · 3 comments
Closed

Support additional context in metrics prefix #1789

asifsmohammed opened this issue Sep 19, 2022 · 3 comments

Comments

@asifsmohammed
Copy link
Member

asifsmohammed commented Sep 19, 2022

Is your feature request related to a problem? Please describe.
Currently PluginMetrics publishes metrics with prefix <pipeline-name>.<plugin-name>.<defined-by-plugins> which adds a restriction to publish any metrics from Data Prepper core peer forwarding or is some cases conditional routing router.

Describe the solution you'd like
Make the pipeline-name component more generic, it can be pipeline name / component-scope. Something like <generic-name>.<component-id>.<defined-metric-name>. So for metrics from core package we could prefix it by <component-scope> instead of a <pipeline-name>.
Examples:

  • log-pipeline.buffer.numberOfRecords
  • log-pipeline.grok.numberOfRecords
  • log-pipeline.opensearch.numberOfRecords
  • log-pipeline.router.defined-metric-name
  • core.peer-forwarder.defined-metric-name

Pros:

  • No need to change any existing code in PluginMetrics class.

Cons:

  • If pipeline name is core all the metrics will be prefixed with core.

Describe alternatives you've considered (Optional)
An alternative could be <pipeline-name>.<component-scope>.<component-id>.<defined-metric-name>.
Examples:

  • log-pipeline.buffer.buffer.numberOfRecords
  • log-pipeline.processor.grok.numberOfRecords
  • log-pipeline.sink.opensearch.numberOfRecords
  • log-pipeline.core.peer-forwarder.defined-metric-name
  • log-pipeline.core.router.defined-metric-name

Cons for alternative approach:

  • Peer forwarder server side metrics might not be aware of the pipeline name in case of bad requests or if the request contains invalid pipeline name.

Additional context

public static PluginMetrics fromNames(final String name, final String pipelineName) {
return new PluginMetrics(new StringJoiner(MetricNames.DELIMITER)
.add(pipelineName)
.add(name).toString());

@asifsmohammed asifsmohammed added this to the v2.0 milestone Sep 20, 2022
@asifsmohammed asifsmohammed added this to Untriaged in (deprecated) Tracking Board via automation Sep 20, 2022
@cmanning09
Copy link
Contributor

Thanks @asifsmohammed, I have a few questions:

What are all the metrics for core peer forwarding we plan on supporting/providing to users? Will there be client side metrics we will want to emit for different aggregate processors? This may help with trying to define the syntax as this changes the requirements.

Can you elaborate on the solution analysis? What are the cons to the first solution and pros to the alternative approach?

@dlvenable
Copy link
Member

I like the solution you proposed. One con is that we could have metric name conflicts if users name their pipeline core. I'd suggest that we disallow pipelines with the names core or data-prepper. In this way, we reserve this metric prefix.

An alternative is to prefix all pipeline metrics with pipeline.<pipeline-name>. I'm not in favor of this because most metrics are this way and it becomes very verbose. Thus, I like the reservation approach personally.

@asifsmohammed
Copy link
Member Author

What are all the metrics for core peer forwarding we plan on supporting/providing to users?

I updated list of metrics for core peer forwarder in the issue here.
#1609 (comment)

(deprecated) Tracking Board automation moved this from Untriaged to Done Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants