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

chore: rename Metric Handle to Bound Instrument #638

Merged

Conversation

@cthulhu-bot
Copy link
Contributor

cthulhu-bot commented Dec 20, 2019

Which problem is this PR solving?

Short description of the changes

Updated references of:

  • handle -> instrument
  • getHandle -> bind
  • removeHandle -> unbind
  • getDefaultHandle -> getDefaultBound

The spec appears to make a distinction between instrument and bound instrument:
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics-user.md#metric-calling-conventions

But I couldn't find any delineation separating these 2 types in this repo. Hence the change of Handle to BoundInstrument and all other subtypes defined as instruments (Counter, Gauge, etc).

Further down it also present the concept of Binding as just a method on the individual instrument types: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics-user.md#bound-instrument-api

So maybe the BoundInstruments file in opentelemetry-metrics should be named Instrument instead?

Please let me know if I missed anything in the rename or renamed something that shouldn't have been.

Thank you!

@cthulhu-bot cthulhu-bot force-pushed the cthulhu-bot:rename-handler-to-instrument branch from 6aaa73f to 2836cee Dec 20, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 20, 2019

Codecov Report

Merging #638 into master will decrease coverage by 0.19%.
The diff coverage is 96.85%.

@@            Coverage Diff            @@
##           master     #638     +/-   ##
=========================================
- Coverage   89.88%   89.68%   -0.2%     
=========================================
  Files         213      212      -1     
  Lines       10234    10065    -169     
  Branches      933      935      +2     
=========================================
- Hits         9199     9027    -172     
- Misses       1035     1038      +3
Impacted Files Coverage Δ
packages/opentelemetry-metrics/src/Meter.ts 96.96% <100%> (ø) ⬆️
...emetry-exporter-prometheus/test/prometheus.test.ts 98.6% <100%> (-0.01%) ⬇️
.../opentelemetry-core/test/metrics/NoopMeter.test.ts 97.22% <100%> (ø) ⬆️
packages/opentelemetry-metrics/test/Meter.test.ts 100% <100%> (ø) ⬆️
...-metrics/test/export/ConsoleMetricExporter.test.ts 100% <100%> (ø) ⬆️
...kages/opentelemetry-metrics/src/BoundInstrument.ts 95.55% <100%> (ø)
...ckages/opentelemetry-core/src/metrics/NoopMeter.ts 80.76% <81.48%> (ø) ⬆️
packages/opentelemetry-metrics/src/Metric.ts 89.09% <95.65%> (ø) ⬆️
...kages/opentelemetry-plugin-dns/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...ages/opentelemetry-plugin-http/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
... and 27 more
@cthulhu-bot cthulhu-bot force-pushed the cthulhu-bot:rename-handler-to-instrument branch from 2836cee to 9e8d389 Dec 20, 2019
@cthulhu-bot cthulhu-bot force-pushed the cthulhu-bot:rename-handler-to-instrument branch from 9e8d389 to 343294f Dec 20, 2019
@cthulhu-bot cthulhu-bot force-pushed the cthulhu-bot:rename-handler-to-instrument branch 3 times, most recently from 8ba8768 to df15b31 Dec 21, 2019
Copy link
Contributor

dyladan left a comment

I was going through and commenting on these and I noticed a trend where you renamed handle => instrument which is not accurate. All metrics are instruments even if they are not bound. The renaming that should have been done was handle to boundInstrument or binding depending on context.

const labelSet = meter.labels({ route: req.path });
const handle = requestCount.getHandle(labelSet);
handles.set(req.path, handle);
const instrument = requestCount.bind(labelSet);

This comment has been minimized.

Copy link
@dyladan

dyladan Dec 23, 2019

Contributor

Can we rename this boundInstrument and the map boundInstruments?

This comment has been minimized.

Copy link
@cthulhu-bot

cthulhu-bot Dec 23, 2019

Author Contributor

How about boundCounter and the map boundInstruments?

@@ -253,7 +253,7 @@ app.use(countAllRequests());

Now, when we make requests to our service our meter will count all requests.

**Note**: Creating a new `labelSet` and `handle` on every request is not ideal as creating the `labelSet` can often be an expensive operation. This is why handles are created and stored in a `Map` according to the route key.
**Note**: Creating a new `labelSet` and `instrument` on every request is not ideal as creating the `labelSet` can often be an expensive operation. This is why instruments are created and stored in a `Map` according to the route key.

This comment has been minimized.

Copy link
@dyladan

dyladan Dec 23, 2019

Contributor
Suggested change
**Note**: Creating a new `labelSet` and `instrument` on every request is not ideal as creating the `labelSet` can often be an expensive operation. This is why instruments are created and stored in a `Map` according to the route key.
**Note**: Creating a new `labelSet` and `binding` on every request is not ideal as creating the `labelSet` can often be an expensive operation. This is why bound instruments are created and stored in a `Map` according to the route key.
const labelSet = meter.labels({ route: req.path });
const handle = requestCount.getHandle(labelSet);
handles.set(req.path, handle);
const instrument = requestCount.bind(labelSet);

This comment has been minimized.

Copy link
@dyladan

dyladan Dec 23, 2019

Contributor

pls rename boundInstrument

This comment has been minimized.

Copy link
@cthulhu-bot

cthulhu-bot Dec 23, 2019

Author Contributor

boundCounter?

const labelSet = meter.labels({ route: req.path });
const handle = requestCount.getHandle(labelSet);
handles.set(req.path, handle);
const instrument = requestCount.bind(labelSet);

This comment has been minimized.

Copy link
@dyladan

dyladan Dec 23, 2019

Contributor

pls rename boundInstrument

This comment has been minimized.

Copy link
@cthulhu-bot

cthulhu-bot Dec 23, 2019

Author Contributor

boundCounter?

/** A Handle for Counter Metric. */
export interface CounterHandle {
/** An Instrument for Counter Metric. */
export interface CounterInstrument {

This comment has been minimized.

Copy link
@dyladan

dyladan Dec 23, 2019

Contributor

I think I would rather call this something like BoundCounter

This comment has been minimized.

Copy link
@cthulhu-bot

cthulhu-bot Dec 23, 2019

Author Contributor

I'll also rename the others to BoundGauge and BoundMeasure

}

/**
* Removes the Handle from the metric, if it is present.
* @param labels the canonicalized LabelSet used to associate with this metric handle.
* Removes the Instrument from the metric, if it is present.

This comment has been minimized.

Copy link
@dyladan

dyladan Dec 23, 2019

Contributor

Binding

Suggested change
* Removes the Instrument from the metric, if it is present.
* Removes the Binding from the metric, if it is present.
}
}
}

export class NoopCounterHandle implements CounterHandle {
export class NoopCounterInstrument implements CounterInstrument {

This comment has been minimized.

Copy link
@dyladan

dyladan Dec 23, 2019

Contributor

I think we need to call this something like NoopBoundCounter

@@ -165,13 +171,17 @@ export class NoopMeasureHandle implements MeasureHandle {
}
}

export const NOOP_GAUGE_HANDLE = new NoopGaugeHandle();
export const NOOP_GAUGE_METRIC = new NoopGaugeMetric(NOOP_GAUGE_HANDLE);
export const NOOP_GAUGE_INSTRUMENT = new NoopGaugeInstrument();

This comment has been minimized.

Copy link
@dyladan

dyladan Dec 23, 2019

Contributor

I think this needs to be named like NOOP_BOUND_GAUGE


const handle = counter.getHandle(meter.labels({ key1: 'labelValue1' }));
handle.add(10);
const instrument = counter.bind(meter.labels({ key1: 'labelValue1' }));

This comment has been minimized.

Copy link
@dyladan

dyladan Dec 23, 2019

Contributor

rename boundInstrument

@cthulhu-bot

This comment has been minimized.

Copy link
Contributor Author

cthulhu-bot commented Dec 23, 2019

I was going through and commenting on these and I noticed a trend where you renamed handle => instrument which is not accurate. All metrics are instruments even if they are not bound. The renaming that should have been done was handle to boundInstrument or binding depending on context.

Ok good this was my main question that you answered, because I didn't understand the difference between instrument and boundInstrument just from reading the spec.

@cthulhu-bot cthulhu-bot force-pushed the cthulhu-bot:rename-handler-to-instrument branch from df15b31 to 98e9891 Dec 23, 2019
@dyladan

This comment has been minimized.

Copy link
Contributor

dyladan commented Dec 23, 2019

@cthulhu-bot please do not change history on your branch by doing force push until the code review is complete. It makes it hard to track history.

@cthulhu-bot

This comment has been minimized.

Copy link
Contributor Author

cthulhu-bot commented Dec 23, 2019

@cthulhu-bot please do not change history on your branch by doing force push until the code review is complete. It makes it hard to track history.

Sorry i'm used to amending commits and force pushing

Copy link
Member

mayurkale22 left a comment

lgtm

@dyladan dyladan added this to the Alpha v0.4 milestone Dec 26, 2019
* chore: fix renames in metrics test

* chore: fix formatting

* chore: rename BaseInstrument to BaseBoundInstrument
@cthulhu-bot cthulhu-bot force-pushed the cthulhu-bot:rename-handler-to-instrument branch from 22e047d to bbde5ce Dec 26, 2019
@cthulhu-bot

This comment has been minimized.

Copy link
Contributor Author

cthulhu-bot commented Dec 27, 2019

Last force push was just to squash everything down and rebase on top of the master branch, should be good to go

@dyladan

This comment has been minimized.

Copy link
Contributor

dyladan commented Dec 27, 2019

@cthulhu-bot i'd like to get more @open-telemetry/javascript-approvers to look at this before moving

@mayurkale22

This comment has been minimized.

Copy link
Member

mayurkale22 commented Jan 3, 2020

@open-telemetry/javascript-approvers ping, we need more reviews for this one!

@dyladan

This comment has been minimized.

Copy link
Contributor

dyladan commented Jan 3, 2020

@mayurkale22 how many reviews do you think we need for this? I had hoped @bg451 would have time to look since he wrote the original implementation, but if not I think this is a relatively safe change as I'm not aware of anyone using metrics yet anyways.

@mayurkale22

This comment has been minimized.

Copy link
Member

mayurkale22 commented Jan 3, 2020

Even I was waiting for @bg451's approval, but make sense to merge it now given that we already have three approvals and relatively safe change.

@mayurkale22 mayurkale22 merged commit 6f0084f into open-telemetry:master Jan 3, 2020
5 of 6 checks passed
5 of 6 checks passed
ci/circleci: lint_&_docs Your tests failed on CircleCI
Details
ci/circleci: node10 Your tests passed on CircleCI!
Details
ci/circleci: node12 Your tests passed on CircleCI!
Details
ci/circleci: node12-browsers Your tests passed on CircleCI!
Details
ci/circleci: node8 Your tests passed on CircleCI!
Details
cla/linuxfoundation mayurkale22 authorized
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.