Support `_created` time series suppression #791

mindw · 2022-06-05T11:50:29Z

If PROMETHEUS_DISABLE_CREATED_SERIES environment variable is true suppress creation of _created OpenMetrics time series.

My Java skills are pretty much non-existant, all comments are welcome :)
Fixes #774

When merging, please consider using squash-merging, thanks :)

CC @fstab @SuperQ

Help needed:

~~Place to put documentation~~
~~Also support setting via property?~~

dhoard · 2022-06-06T01:12:54Z

@mindw I took your code and refactored it in an effort to maintain existing performance (remove method calls.)

Please take a look: #793

CC @fstab @SuperQ

mindw · 2022-06-06T08:15:06Z

@dhoard thank you for suggested code changes - they where added to the PR with a minor twist.

Can you please suggest the best place to document this? would it be the README.md? JavaDoc?

It may not be a very good idea exposing any code as public unless its explicitly supposed to be part of the interface.

dhoard · 2022-06-06T12:41:54Z

@mindw We should document the use of the environment variable in the READMD.md.

| It may not be a very good idea exposing any code as public unless its explicitly supposed to be part of the interface.

I agree. Typically, the code would use a static initialization block in Collector to get the environment variable and set a final USE_CREATED variable when the class loads. This is the ideal solution for a configure once scenario. The code change is easy... but I couldn't get the test code working.

mindw · 2022-06-10T11:04:05Z

@dhoard is there anything left preventing this PR form being merged?

CC @fstab

dhoard · 2022-06-10T11:14:51Z

@mindw looks good to me. @fstab will need to review and merge.

fstab

Thanks for the PR. I added a few comments, nothing major. Please have a look.

README.md

fstab · 2022-06-11T20:25:11Z

simpleclient/src/main/java/io/prometheus/client/Collector.java

@@ -150,44 +149,43 @@ public MetricFamilySamples filter(Predicate<String> sampleNameFilter) {
     * {@code # HELP}), and as this name <a href="https://github.com/prometheus/common/issues/319">must be unique</a>
     * we include the name without suffix here as well.
     */
-    public String[] getNames() {
+    public List<String> getNames() {


What about leaving getNames() as it is?

The _created names in getNames() are used to prevent illegal name clashes, like somebody registering a counter named my_counter_total but also a gauge named my_counter_created. These are a weird corner cases, and I think it's strange to allow this when the PROMETHEUS_DISABLE_CREATED_SERIES environment variable is set.

If we leave getNames() as it was, PROMETHEUS_DISABLE_CREATED_SERIES would just enable/disable the _created series. If we change getNames(), we will allow weird corner cases that are usually prevented. As a result, there will be corner cases where an application breaks unless PROMETHEUS_DISABLE_CREATED_SERIES is set. I don't think this is the intention.

#774 (comment) suggests these are actually not forbidden just discouraged. Sure, evil minded integrators may abuse this by developing their code under PROMETHEUS_DISABLE_CREATED_SERIES=true but it will break on the first time it will not be used.

#774 (comment) says that my general statement

it is illegal to use the _created suffix in custom metric names

is incorrect. However, it is still true that OpenMetrics disallows _created metrics when there are potential name clashes:

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.

As the getNames() method is used to prevent these name clashes, I think it would be good to leave it as it is.

Fair enough - I've reverted the this change. Thanks for taking the time and effort explaining it!

fstab · 2022-06-11T20:30:07Z

simpleclient/src/main/java/io/prometheus/client/Collector.java

+  protected static final String DISABLE_CREATED_SERIES = "PROMETHEUS_DISABLE_CREATED_SERIES";
+  private static final List<String> TRUTHS = Arrays.asList("true", "1", "t");
+
+  protected static boolean USE_CREATED = getUseCreated();


Please make it final. It's not supposed to change.

If you make it final but still want to test it, you should not make it static.

Rename to CREATED_SERIES_DISABLED because that makes it clearer that this is related to the PROMETHEUS_DISABLE_CREATED_SERIES environment variable.

If you revert the changes to getNames(), you could move this one level down in the class hierarchy to SimpleCollector.

Please make it final. It's not supposed to change.

If you make it final but still want to test it, you should not make it static.

Yes, it was final - but then I could not find a way to test it. It I remove the static every object will have its own copy of it which seems to be excessive. The above is a compromise.

Rename to CREATED_SERIES_DISABLED because that makes it clearer that this is related to the PROMETHEUS_DISABLE_CREATED_SERIES environment variable.

That will flip it's meaning and will change all code from

if (USE_CREATED) {

to

if (!CREATED_SERIES_DISABLED) {

Which is a double negative and is considered as a poor programming practice. For example - https://cleankotlin.nl/blog/double-negations#:~:text=Double%20negations%20are%20not%20not,can%20hide%20in%20your%20program.

If you revert the changes to getNames(), you could move this one level down in the class hierarchy to SimpleCollector.

Not sure of the benefit - let's resolve getNames() discussion first.

I think if the flag should not change at runtime it should be final, even if that makes it a bit harder to test. There are a few options how to write a test if it's final:

Make it non-static

Set the field via reflection (you can remove the final modifier via reflection)

Write an integration test with Testcontainers test (then you could set the environment variable before starting the test application).

Regarding the name of the flag: It's always hard to maintain code if there are different names for the same concept. If we name the environment variable PROMETHEUS_DISABLE_CREATED_SERIES but the flag USE_CREATED somebody reading the code would always need to double-check if these mean the same thing. If we use a similar name, it's more obvious.

I think if the flag should not change at runtime it should be final, even if that makes it a bit harder to test. There are a few options how to write a test if it's final:

Make it non-static

Set the field via reflection (you can remove the final modifier via reflection)

Write an integration test with Testcontainers test (then you could set the environment variable before starting the test application).

I'm not sure what I can add. The extra variable per object seems like too much a price to pay given the alternative.

Can you please provide some guidance on how to use Testcontainers here? I'm unfamiliar with that framework.
Otherwise it seems like a good way on making the variable final (which something I wanted to do in the first place). It will also make the new dependency redundant.

Regarding the name of the flag: It's always hard to maintain code if there are different names for the same concept. If we name the environment variable PROMETHEUS_DISABLE_CREATED_SERIES but the flag USE_CREATED somebody reading the code would always need to double-check if these mean the same thing. If we use a similar name, it's more obvious.

Thanks for pointing this rule out. We're talking about readability multiple places vs readability one place. For me the many wins - the condition should be read as "TRUE" instead of "NOT NOT". Finally, the this choice mirrors the python one. The environment variable is mentioned once in a function and is pretty easy to lookup in any decent IDE.

fstab · 2022-06-11T20:51:16Z

simpleclient/src/test/java/io/prometheus/client/DisableCreatedTest.java

+
+        List<Collector.MetricFamilySamples> mfsList;
+        try {
+            env.setup();


If you make the flag final, you'll need to call env.setup() before initializing the metrics.

How can I do that?

That was referring to

If you make it final but still want to test it, you should not make it static.

There are a few other options how to write a test though.

As noted above - some guidance/example on how to use the framework would be appreciated.

Have a look at JavaVersionsIT. I guess for your test you won't need different Java versions, but apart from that it should be similar.

fstab · 2022-06-11T20:51:45Z

simpleclient/pom.xml

+      <dependency>
+          <groupId>org.mockito</groupId>
+          <artifactId>mockito-inline</artifactId>
+          <version>3.12.4</version>
+          <scope>test</scope>
+      </dependency>


Not needed?

Alas, it seems to be needed - https://github.com/webcompere/system-stubs#dependencies .

I removed the dependency, and the test works just fine. I don't understand why it is needed.

Anyway, if the system-stubs dependency is hard to understand, it might be good not to use it. A few of the options how to make the code testable do not include modifying environment variables at runtime:

Use reflection to set the flag

Write an integration test with Testcontainers.

Yes, that is the expected output for JVM 11.

Quoting from the link above:

⚠ WARNING: JDK Compatibility. From JDK16 onwards, there are deeper restrictons on the ability to use reflection. Previous versions of this library, and others in the space, encounter an Illegal Reflective Access warning, or even a runtime error such as java.lang.reflect.InaccessibleObjectException when trying to manipulate the Map behind the system's environment variables.

Consequently, this library now uses mockito-inline version 3.x to enable the interception of calls for reading environment variables. This requires consumers to both use a compatible version of Mockito AND be prepared for the inline implementation of Mockito mocks.

Note: Groovy users may need Mockito >= 4.5.0 for compatibility.

As noted above - some guidance/example on how to use the Testcontainers will make this discussion moot.

…ed` time series Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

fstab · 2022-06-13T17:15:21Z

@mindw I think I'll do a release this week, and it would be good to include this feature. What do you think about adding the feature now (and make the flag final), and continue with the test later in a separate PR? That way the feature would make it into the release, and we'd have more time to get the test done.

mindw · 2022-06-13T18:46:01Z

What do you think about adding the feature now (and make the flag final), and continue with the test later in a separate PR? That way the feature would make it into the release, and we'd have more time to get the test done.

No problem - tests will wait ill the weekend

Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

fstab · 2022-06-14T11:11:51Z

Thanks a lot @mindw. One last thing: I'm wondering whether Collector is the right place for reading the environment variable. Initially I was thinking to move it up to SimpleCollector, because it's only relevant for Counter, Histogram, and Summary, and not for other collectors.

However, thinking about it I feel that it might be worthwhile to move the environment variable parsing into its own class, like this:

class EnvironmentVariables {

    private static final String DISABLE_CREATED_SERIES = "PROMETHEUS_DISABLE_CREATED_SERIES";
    private static final List<String> TRUTHS = Arrays.asList("true", "1", "t");
    private static final boolean createdSeriesEnabled = !isTrue(DISABLE_CREATED_SERIES);

    static boolean isCreatedSeriesEnabled() {
        return createdSeriesEnabled;
    }

    private static boolean isTrue(String envVarName) {
        String stringValue = System.getenv(envVarName);
        if (stringValue != null) {
            return TRUTHS.contains(stringValue.toLowerCase());
        }
        return false;
    }
}

Then the Histogram, Counter, and Summary could use it like this:

if (EnvironmentVariables.isCreatedSeriesEnabled()) {
    // add _created sample
}

I feel this would help with separation of concerns. What do you think?

dhoard · 2022-06-14T11:32:49Z

@fstab @mindw I personally like...

class Environment {

    private static final String DISABLE_CREATED_SERIES = "PROMETHEUS_DISABLE_CREATED_SERIES";
    private static final List<String> DISABLE_CREATED_SERIES_TRUE = Arrays.asList("true", "1", "t");
    private static final boolean includeCreatedSeries = !isTrue(DISABLE_CREATED_SERIES);

    static boolean includeCreatedSeries() {
        return includeCreatedSeries;
    }

    private static boolean isTrue(String envVarName) {
        String stringValue = System.getenv(envVarName);
        if (stringValue != null) {
            return DISABLE_CREATED_SERIES_TRUE.contains(stringValue.toLowerCase());
        }
        return false;
    }
}

Since it's really environment configuration.

Then the Histogram, Counter, and Summary could use it like this:

if (Environment.includeCreatedSeries()) {
	// add _created series
}

Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

mindw · 2022-06-15T05:35:33Z

@fstab It is done :) It seems to me that both suggestions converged.
Cheers!

fstab · 2022-06-15T08:43:10Z

👍 thanks!

mindw force-pushed the suppress_created branch from 175fec6 to 1930110 Compare June 5, 2022 11:52

mindw force-pushed the suppress_created branch from c9475f9 to 87ee945 Compare June 6, 2022 08:09

mindw force-pushed the suppress_created branch from d45d56a to 47c02f0 Compare June 7, 2022 06:47

fstab requested changes Jun 11, 2022

View reviewed changes

fstab mentioned this pull request Jun 11, 2022

Support _created time series suppression #793

Closed

mindw force-pushed the suppress_created branch from 2f1dfaf to d4853be Compare June 12, 2022 05:42

mindw added 4 commits June 12, 2022 08:42

Support PROMETHEUS_DISABLE_CREATED_SERIES env var to suppress `_creat…

f6bc10c

…ed` time series Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

switched to use a static variable. test tweaks.

35ffae3

Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

docs

6288a32

Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

let's be truer.

b3de1f9

Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

mindw force-pushed the suppress_created branch from d4853be to b3de1f9 Compare June 12, 2022 05:43

Revert GetNames() redundant change.

744c906

Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

mindw force-pushed the suppress_created branch from f9731b8 to 744c906 Compare June 13, 2022 11:39

make final, drop test for now

d59bfdc

Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

mindw force-pushed the suppress_created branch from 867194a to d59bfdc Compare June 13, 2022 19:35

remove the extra deps too.

2fe6737

Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

Add a brand new and shiny class

40b4206

Signed-off-by: Gabi Davar <grizzly.nyo@gmail.com>

fstab merged commit 6730f3e into prometheus:master Jun 15, 2022

mindw deleted the suppress_created branch June 15, 2022 11:14

JnRouvignac mentioned this pull request Aug 18, 2022

allowing to keep invalid counter names (with no _total suffix) #653

Closed

risdenk mentioned this pull request Mar 2, 2023

Update io.prometheus:* to v0.16.0 apache/solr#1419

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `_created` time series suppression #791

Support `_created` time series suppression #791

mindw commented Jun 5, 2022 •

edited

dhoard commented Jun 6, 2022

mindw commented Jun 6, 2022

dhoard commented Jun 6, 2022

mindw commented Jun 10, 2022 •

edited

dhoard commented Jun 10, 2022

fstab left a comment

fstab Jun 11, 2022 •

edited

mindw Jun 12, 2022 •

edited

fstab Jun 13, 2022

mindw Jun 13, 2022

fstab Jun 11, 2022 •

edited

mindw Jun 12, 2022 •

edited

fstab Jun 13, 2022

mindw Jun 13, 2022

fstab Jun 11, 2022

mindw Jun 12, 2022

fstab Jun 13, 2022

mindw Jun 13, 2022

fstab Jun 13, 2022

fstab Jun 11, 2022

mindw Jun 12, 2022

fstab Jun 13, 2022

mindw Jun 13, 2022 •

edited

fstab commented Jun 13, 2022

mindw commented Jun 13, 2022

fstab commented Jun 14, 2022 •

edited

dhoard commented Jun 14, 2022

mindw commented Jun 15, 2022

fstab commented Jun 15, 2022

Support _created time series suppression #791

Support _created time series suppression #791

Conversation

mindw commented Jun 5, 2022 • edited

dhoard commented Jun 6, 2022

mindw commented Jun 6, 2022

dhoard commented Jun 6, 2022

mindw commented Jun 10, 2022 • edited

dhoard commented Jun 10, 2022

fstab left a comment

Choose a reason for hiding this comment

fstab Jun 11, 2022 • edited

Choose a reason for hiding this comment

mindw Jun 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fstab Jun 11, 2022 • edited

Choose a reason for hiding this comment

mindw Jun 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mindw Jun 13, 2022 • edited

Choose a reason for hiding this comment

fstab commented Jun 13, 2022

mindw commented Jun 13, 2022

fstab commented Jun 14, 2022 • edited

dhoard commented Jun 14, 2022

mindw commented Jun 15, 2022

fstab commented Jun 15, 2022

Support `_created` time series suppression #791

Support `_created` time series suppression #791

mindw commented Jun 5, 2022 •

edited

mindw commented Jun 10, 2022 •

edited

fstab Jun 11, 2022 •

edited

mindw Jun 12, 2022 •

edited

fstab Jun 11, 2022 •

edited

mindw Jun 12, 2022 •

edited

mindw Jun 13, 2022 •

edited

fstab commented Jun 14, 2022 •

edited