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

Allow passing counter metric name to datapoint to show in case of validation error #1121

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AntuanRokanten
Copy link

@@ -1,19 +1,30 @@
package io.prometheus.metrics.model.snapshots;

public abstract class DataPointSnapshot {
private final String metricName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add https://mvnrepository.com/artifact/com.google.code.findbugs/jsr305/3.0.2 (complile only) and annotate with @Nullable

this(null, value, labels, exemplar, createdTimestampMillis, scrapeTimestampMillis);
}

public CounterDataPointSnapshot(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think builder is enough

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what you mean here, please elaborate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need an overloaded ctor - you can just use the builder to pass the metric name

@@ -105,12 +116,17 @@ public Builder createdTimestampMillis(long createdTimestampMillis) {
return this;
}

public Builder metricName(String metricName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

javadoc please

@zeitlinger
Copy link
Member

started to refactor in main...pass-metric-name-to-datapoint

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 this pull request may close these issues.

2 participants