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.

Sign up for GitHub

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

Make Labels Optional for CounterMetric::add #1032

Merged
merged 12 commits into from
May 13, 2020
Merged

Make Labels Optional for CounterMetric::add #1032

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2cb9dae
Make labels optional for add
astorm May 7, 2020
abf279c
updating interfaces, and MeasureMetric
astorm May 7, 2020
0c4c82b
lint
astorm May 7, 2020
276fedf
for a metri defined with label keys, but without metric values
astorm May 7, 2020
fb000c1
whatever you say lint
astorm May 7, 2020
9928ba7
Merge branch 'master' into astorm/labels-optional
astorm May 8, 2020
15a1cf7
reverting "getValues" changes
astorm May 8, 2020
649a4b8
Merge branch 'master' into astorm/labels-optional
astorm May 8, 2020
d12dbda
Merge branch 'master' into astorm/labels-optional
astorm May 12, 2020
38cb3a9
adding shoulder, fixing param
astorm May 13, 2020
ffd2593
Merge branch 'master' into astorm/labels-optional
astorm May 13, 2020
daa44cd
Merge branch 'master' into astorm/labels-optional
dyladan May 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/opentelemetry-api/src/metrics/Metric.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,14 @@ export interface Counter extends UnboundMetric<BoundCounter> {
/**
* Adds the given value to the current value. Values cannot be negative.
*/
add(value: number, labels: Labels): void;
add(value: number, labels?: Labels): void;
}

export interface Measure extends UnboundMetric<BoundMeasure> {
/**
* Records the given value to this measure.
*/
record(value: number, labels: Labels): void;
record(value: number, labels?: Labels): void;

record(
value: number,
Expand Down
4 changes: 3 additions & 1 deletion packages/opentelemetry-exporter-prometheus/src/prometheus.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,9 @@ export class PrometheusExporter implements MetricExporter {
*/
if (metric instanceof Counter) {
metric.remove(
...record.descriptor.labelKeys.map(k => record.labels[k].toString())
...record.descriptor.labelKeys.map(k =>
Copy link
Member

Choose a reason for hiding this comment

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

In what cases would record.labels[k] be missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my manual testing last night this change prevented an error being thrown when a user instantiates a counter with a label key

const monotonicCounter = meter.createCounter('monotonic_counter', {
  monotonic: true,
  labelKeys: ['pid'],
  description: 'Example of a monotonic counter',
});

but then fails to provide a value for that key.

monotonicCounter.add(1, {})
monotonicCounter.add(1)

@dyladan With my build issues sorted/understood I can (depending on how your conversation with @mayurkale22 goes) either add tests that make the use-case here clear or revert this.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I understand. Instead of falling back to an empty string, I think I would prefer a filter on the list.

      metric.remove(
        ...record.descriptor.labelKeys
          .filter(k => record.labels[k] != null)
          .map(k => record.labels[k].toString())
      );

Copy link
Member

Choose a reason for hiding this comment

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

This is clever solution but feel like it won't work as expected.

AFAIK prom-client will throw "Invalid number of arguments" exception if length of keys != length of values. See: https://github.com/siimon/prom-client/blob/master/lib/util.js#L34

One option is to fall back on "value missing or unknown" as a default value when the corresponding value is not defined in any given key(s), similar to the description field. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Option 2: Instead of blindly attaching our Labelkeys to prom-client's labelNames, we can filter the keys based on valid value.

This is what python has implemented: https://github.com/open-telemetry/opentelemetry-python/blob/master/ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py#L146-L150

Copy link
Contributor Author

@astorm astorm May 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your feedback @mayurkale22

WDYT?

IT I've been wrangled into an bug fix that is just outside the scope of my original PR -- but I can live with that :)

Re: option 2 -- when you say

Option 2: Instead of blindly attaching our Labelkeys to prom-client's labelNames, we can filter the keys based on valid value.

My interpretation of this is it's a suggestion that, when it comes time to create the Prometheus metric, we look at the keys and values of the passed in Open Telemetry metric, remove any keys with "non-valid" values (undefined, null, -- others?) and create the Prometheus metric with this new, smaller, set of keys.

If that's the suggestion -- I don't think this is the right course of action. Label keys seem like a fundamental part of a metric. That is -- a metric with the keys [foo, bar, baz] seems like a different metric than one with the keys [foo, bar]. Removing keys doesn't seems like the right course of action.

Give than, Option 1 seems like the better approach.

One option is to fall back on "value missing or unknown" as a default value when the corresponding value is not defined in any given key(s),

You didn't indicate whether you thought this value should be added when handling the Prometheus metric, or when recording the Open Telemetry metric. My assumption is the former, as having this extra work happen every time a metric is recorded would be excessive.

Given all that: I intend to change how Prometheus metrics are instantiated. If a value isn't present for a specific key, I will use a constant string of value missing for its value. I will add tests to cover this case. I'll check the spec for what constitutes a valid values, and make judgment call if there are ambiguities there.

Copy link
Member

Choose a reason for hiding this comment

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

@dyladan would like you know your thoughts on this?

IT I've been wrangled into an bug fix that is just outside the scope of my original PR

Perhaps we can merge this PR and address prometheus exporter issue in the separate PR (btw, this won't make master branch unstable but we need to apply fix before next release). If we decide to merge, I would suggest to revert prometheus related to changes from this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Think I would rather see it as two separate PRs simply for the purpose of having better history later when we have to go spelunking through PRs to discover why decisions were made. Not a strong preference though. Agree the prom changes should be reverted if this is going to merge without a proper fix.

record.labels[k] ? record.labels[k].toString() : ''
)
);
}

Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-metrics/src/Metric.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export class CounterMetric extends Metric<BoundCounter> implements api.Counter {
* @param labels key-values pairs that are associated with a specific metric
astorm marked this conversation as resolved.
Show resolved Hide resolved
* that you want to record.
*/
add(value: number, labels: api.Labels) {
add(value: number, labels: api.Labels = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

This is nice, it would be nice if you can do the same thing to measure.record (https://github.com/astorm/opentelemetry-js/blob/astorm/labels-optional/packages/opentelemetry-metrics/src/Metric.ts#L166)

Copy link
Member

Choose a reason for hiding this comment

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

Just realized, Prometheus exporter would fail after this change. Especially below line due to toString method on undefined:

opentelemetry-js/packages/opentelemetry-exporter-prometheus/src/prometheus.ts

Line 187 in c454d6c

...record.descriptor.labelKeys.map(k => record.labels[k].toString())

I would prefer to fix it in this same PR and do some end to end manual testing. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Also, in order to pass the build you need to make the same change in the API package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the guidance and patience @mayurkale22 -- I'm new to the repo and the project and I'm still getting my sea legs.

Fixing this for the MeasureMetric in this PR makes sense. Fixing the prometheus exporter in this PR also makes sense. I presume the change in opentelemetry-api would be to the interfaces, and that also makes sense to do in this PR.

I'll ping -- you? -- via a PR comment when that's done. If there's more to The Process™ please let me know.

Best wishes.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM, just add a comment on this thread when new changes are ready to review. I usually use this example to test against prom. exporter.

Copy link
Member

Choose a reason for hiding this comment

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

@mayurkale22 not sure why you think the prometheus exporter would fail on that line. Can you explain? I'm hesitant to approve changes to the _registerMetric routine in the prometheus exporter because the prom-client has some surprising behavior, and that is the logic we use to destroy and recreate the counters with the new values.

Copy link
Member

Choose a reason for hiding this comment

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

@mayurkale22 not sure why you think the prometheus exporter would fail on that line. Can you explain?

Looks like @astorm already mention the reason, here-> #1032 (comment) (due to toString method on undefined label value).

I'm hesitant to approve changes to the _registerMetric routine in the prometheus exporter because the prom-client ....

I would suggest to add new test in prom. exporter with some LabelKeys but w/o Labels (values). You can reuse existing test (like this one), just dont bind the labels to metric.

this.bind(labels).add(value);
}
}
Expand Down Expand Up @@ -163,7 +163,7 @@ export class MeasureMetric extends Metric<BoundMeasure> implements api.Measure {
);
}

record(value: number, labels: api.Labels) {
record(value: number, labels: api.Labels = {}) {
this.bind(labels).record(value);
}
}
Expand Down
13 changes: 13 additions & 0 deletions packages/opentelemetry-metrics/test/Meter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,19 @@ describe('Meter', () => {
);
});

it('be able to call add with no labels', () => {
astorm marked this conversation as resolved.
Show resolved Hide resolved
const counter = meter.createCounter('name', {
description: 'desc',
unit: '1',
disabled: false,
monotonic: true,
});
counter.add(1);
meter.collect();
const [record1] = meter.getBatcher().checkPointSet();
assert.strictEqual(record1.aggregator.toPoint().value, 1);
});

it('should return counter with resource', () => {
const counter = meter.createCounter('name') as CounterMetric;
assert.ok(counter.resource instanceof Resource);
Expand Down