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 7 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: 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