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

Add ability to turn off _created metrics #774

Closed
mindw opened this issue Apr 7, 2022 · 16 comments · Fixed by #791
Closed

Add ability to turn off _created metrics #774

mindw opened this issue Apr 7, 2022 · 16 comments · Fixed by #791

Comments

@mindw
Copy link
Contributor

mindw commented Apr 7, 2022

Clone of prometheus/client_python#672

As per the June 24th 2021 Prometheus Dev Summit there is consensus that all first party clients should have a parameter to turn off _created metrics.

@fstab
Copy link
Member

fstab commented Apr 7, 2022

Thanks @mindw. There is a SampleNameFilter with the following default implementations:

  • name is equal to
  • name is not equal to
  • name starts with
  • name does not start with

This can be configured programmatically, and in the exporter Servlet it can also be configured via Servlet configuration.

What do you think about adding

  • name must end with
  • name must not end with

Then you could just say nameMustNotEndWith("_created") to get rid of all _created metrics.

Would that work? It would be trivial to implement :)

@mindw
Copy link
Contributor Author

mindw commented Apr 7, 2022

Thanks for the pointers @fstab !
But perhaps it should match the Python implementation prometheus/client_python#774- namely use an environment variable (PROMETHEUS_DISABLE_CREATED_SERIES) to suppress them? The rational stated in it makes sense to me.

Also, I would like to implement @SuperQ suggestion of making it an option of the registry.

Yes, even with my feeble Java skills it should be easy implement and test.

@fstab
Copy link
Member

fstab commented Apr 7, 2022

Currently, the SampleNameFilter can be configured different places, depending on how you want to expose metrics:

  • If you expose metrics using the built-in HTTPServer, you can set it in HTTPServer.Builder.
  • If you expose metrics using the exporter Servlet, you can configure it in the servlet configuration.

In the unlikely case that you write your own exporter, you can pass the filter as a parameter to CollectorRegistry.filteredMetricFamilySamples() in the implementation of the scrape handler of your exporter.

I think this covers all cases, and a new option for the registry is not needed in client_java.

We should extend the SampleNameFilter with methods

  • nameEndsWith()
  • nameDoesNotEndWith()

and add this to the configuration in HTTPServer.Builder and in the exporter Servlet.

PROMETHEUS_DISABLE_CREATED_SERIES would be a shortcut for saying nameDoesNotEndWith("_created"). I'm not sure if it would be good to add an environment variable for that. I know some users filter out metrics starting with jvm_threads_deadlocked, so why not implement a PROMETHEUS_DISABLE_JVM_THREADS_DEADLOCKED_SERIES environment variable? I feel if we take that path, we'll end up with an arbitrary collection of filters that can be configured via environment variables. I prefer using the generic name filter configuration that we have now.

That being said, if other libraries like client_golang adopt PROMETHEUS_DISABLE_CREATED_SERIES and it becomes common among client libraries, client_java will support it.

@mindw
Copy link
Contributor Author

mindw commented Apr 7, 2022

@fstab thank you for your valuable inputs. While the suggested implementation you've suggested seems perfectly reasonable to me, the discussion here isn't about the implementation details.

I'll try to summarize the referenced tickets above.

IMHO the rational for choosing an environment variable strategy for implementation _created metrics is to allow disabling those metrics at runtime without modifying the code. The effort for changing the code of all counters is significantly larger than bumping the library version. This is aimed at the DevOps engineers maintaining production environment - they can reduce the load on the Prometheus server by adding environment variables. The alternative is to go exporter by exporter and make PRs for every exporter - a far more time consuming process.

@SuperQ suggestion makes sense as it allows the exporter implementer to opt out at the registry - this is useful for exporters which re-create all of the metrics every time they are requested.

Hopefully the above makes sense.

@fstab
Copy link
Member

fstab commented Apr 7, 2022

I think there is a misunderstanding. I wrote above

In the unlikely case that you write your own exporter, you can pass the filter as a parameter to CollectorRegistry.filteredMetricFamilySamples() in the implementation of the scrape handler of your exporter.

This is a method of the registry. There is no need to change code of all counters.

@dhoard
Copy link
Collaborator

dhoard commented May 19, 2022

Adding the use of an environment variable in a client library doesn't feel like the correct approach to me.

I think adding a SampleNameFilter that gets applied to every collection makes more sense...

SampleNameFilter sampleNameFilter = new ExcludeCreatedSampleNameFilter();

CollectorRegistry collectorRegistry = CollectorRegistry.default;
collectorRegistry.addFilter(sampleNameFilter);

// ExcludeCreatedSampleNameFilter will get applied automatically as part of the collection
Enumeration<Collector.MetricFamilySamples> metricFamilySamplesEnumeration = collectorRegistry.metricFamilySamples() ;

... and leave the configuration and management of any filters to the exporter/calling code.

@mindw
Copy link
Contributor Author

mindw commented May 20, 2022

@dhoard please see my comment above:

IMHO the rational for choosing an environment variable strategy for implementation _created metrics is to allow disabling those metrics at runtime without modifying the code. The effort for changing the code of all counters is significantly larger than bumping the library version. This is aimed at the DevOps engineers maintaining production environment - they can reduce the load on the Prometheus server by adding environment variables. The alternative is to go exporter by exporter and make PRs for every exporter - a far more time consuming process.

@dhoard
Copy link
Collaborator

dhoard commented May 20, 2022

@mindw Java-based exporters package a specific version of client_java as part of their build/release... so PRs and new releases for every Java-based exporter would still be required.

@fstab
Copy link
Member

fstab commented May 20, 2022

I created #787 so you can see how this looks like using the sample name filter. Comments welcome!

@mindw
Copy link
Contributor Author

mindw commented May 21, 2022

@fstab that is an interesting approach - but IMHO it does not entirely cover the use case.
This isn't about dropping all _created metrics, it is about stopping the library for adding new metrics named *_created implicitly for every counter.
@dhoard yes, some change is required - but there is a difference between bumping client version and forcing the code maintainer to decide whether to keep the _created metrics or not. Especially that there isn't a consensus about it.

IMO Using an environment variable to toggle _created metrics addition for counters is orthogonal to #787 and other suggestions discussed above.

@fstab
Copy link
Member

fstab commented May 22, 2022

This isn't about dropping all _created metrics, it is about stopping the library for adding new metrics named *_created implicitly for every counter.

As per OpenMetrics Spec it is illegal to use the _created suffix in custom metric names:

The name of a MetricFamily MUST NOT result in a potential clash for sample metric names as per the ABNF with another MetricFamily in the Text Format within a MetricSet. An example would be a gauge called "foo_created" as a counter called "foo" could create a "foo_created" in the text format.

So it does not make a difference whether we remove all _created metrics, or just the implicitly generated _created metrics, because there are no custom _created metrics.

@mindw
Copy link
Contributor Author

mindw commented May 22, 2022

@fstab I stand corrected on the user _created point. Thanks for clearing this up! 😔

IMHO both approaches should be implemented:

  1. toggle via environment variable.
  2. toggle pragmatically.

It would be also desirable to be consistent between client libraries (PROMETHEUS_DISABLE_CREATED_SERIES).

For reference see prometheus/client_python#774 (comment) .

@SuperQ
Copy link
Member

SuperQ commented May 22, 2022

As much as I dislike ENV var side-effect behavior, I agree with @mindw in this case. Having both is useful in this case. I personally prefer the code toggle, but as pointed out in the Python implementation, sometimes you're deploying code you can't easily control.

@fstab
Copy link
Member

fstab commented May 22, 2022

Thanks for the discussion so far, I really appreciate your opinions.

If we implement configuration via environment variables, we need to decide whether we should implement a specific variable for disabling _created metrics (like PROMETHEUS_DISABLE_CREATED_SERIES), or generic variables for filtering time series by name.

Examples:

  • This PR in jmx_exporter suggests a toggle for disabling all default exports (the time series prefixed with jvm_).
  • Some people want to disable jvm_threads_deadlocked and jvm_threads_deadlocked_monitor for performance reasons.

With generic name filter environment variables we could provide a mechanism to cover all these cases. For example:

  • PROMETHEUS_FILTER_NAME_MUST_NOT_END_WITH=_created disable _created metrics
  • PROMETHEUS_FILTER_NAME_MUST_NOT_START_WITH=jvm_ disable all default exports, as they are prefixed jvm_.
  • PROMETHEUS_FILTER_NAME_MUST_BE_EQUAL_TO=jvm_threads_deadlocked,jvm_threads_deadlocked_monitor disable two specific metrics.

My feeling is that if we can implement generic name filter variables that cover many scenarios that's better than a specific PROMETHEUS_DISABLE_CREATED_SERIES variable. What do you think?

@SuperQ
Copy link
Member

SuperQ commented May 22, 2022

We only need the one feature, PROMETHEUS_DISABLE_CREATED_SERIES. Nothing more complicated. The reason for the feature is to toggle off an OpenMetrics specific feature that Prometheus doesn't support.

The other things seem to be about filtering collection of metrics, not related to generating meta-series.

@brian-brazil
Copy link
Contributor

As per OpenMetrics Spec it is illegal to use the _created suffix in custom metric names:

This is incorrect. It is discouraged as it may cause confusion, but it's only forbidden to cause potential clashes with other exposed metrics.

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 a pull request may close this issue.

5 participants