feat: implement named meter #691

xiao-lix · 2020-01-13T22:07:50Z

Which problem is this PR solving?

Closes Implement Named Meters #680

Short description of the changes

Implement named meters and update documentation

dyladan · 2020-01-13T22:09:53Z

There is no reason to call the registry the "basic" registry since there is no difference between a node meter and a web meter. That distinction is only for tracing as far as I'm aware.

mayurkale22 · 2020-01-13T22:12:53Z

packages/opentelemetry-types/src/metrics/meter_registry.ts

@@ -0,0 +1,29 @@
+/*!


nit: meter_registry.ts => MeterRegistry.ts

mayurkale22 · 2020-01-13T22:14:20Z

packages/opentelemetry-metrics/src/BasicMeterRegistry.ts

+/**
+ * This class represents a basic tracer registry which platform libraries can extend
+ */
+export class BasicMeterRegistry implements types.MeterRegistry {


I would prefer to keep MeterRegistry instead of BasicMeterRegistry, I don;t expect to see NodeMeterRegistry or WebMeterRegistry.

mayurkale22 · 2020-01-13T22:14:29Z

packages/opentelemetry-metrics/src/BasicMeterRegistry.ts

+import { DEFAULT_CONFIG, MeterConfig } from './types';
+
+/**
+ * This class represents a basic tracer registry which platform libraries can extend


Suggested change

* This class represents a basic tracer registry which platform libraries can extend

* This class represents a basic meter registry which platform libraries can extend

codecov-io · 2020-01-13T22:19:06Z

Codecov Report

Merging #691 into master will decrease coverage by 0.18%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #691      +/-   ##
==========================================
- Coverage   89.89%   89.71%   -0.19%     
==========================================
  Files         217      216       -1     
  Lines       10276    10046     -230     
  Branches      940      936       -4     
==========================================
- Hits         9238     9013     -225     
+ Misses       1038     1033       -5

Impacted Files	Coverage Δ
packages/opentelemetry-metrics/test/Meter.test.ts	`100% <ø> (ø)`	⬆️
...emetry-exporter-prometheus/test/prometheus.test.ts	`98.6% <100%> (ø)`	⬆️
.../opentelemetry-core/test/metrics/NoopMeter.test.ts	`97.29% <100%> (ø)`	⬆️
...s/opentelemetry-metrics/test/MeterRegistry.test.ts	`100% <100%> (ø)`	⬆️
...ackages/opentelemetry-metrics/src/MeterRegistry.ts	`100% <100%> (ø)`	⬆️
...-metrics/test/export/ConsoleMetricExporter.test.ts	`100% <100%> (ø)`	⬆️
...ges/opentelemetry-tracing/src/NoopSpanProcessor.ts	`66.66% <0%> (-8.34%)`	⬇️
...ckages/opentelemetry-core/src/common/NoopLogger.ts	`50% <0%> (-7.15%)`	⬇️
packages/opentelemetry-plugin-mysql/src/utils.ts	`90.9% <0%> (-4.55%)`	⬇️
...opentelemetry-core/src/trace/globaltracer-utils.ts	`90% <0%> (-1.67%)`	⬇️
... and 25 more

…ry-js into xiao/meter-registry

mayurkale22 · 2020-01-13T22:38:25Z

packages/opentelemetry-metrics/src/MeterRegistry.ts

+  }
+
+  getMeter(
+    name: string = 'default',


This is same as open-telemetry/opentelemetry-specification#407 and #679, I would suggest to remove default for now and assign the names in examples and tests. We can come back to this and update based on the decision. @xiao-lix and @dyladan WDYT?

Definitely name should be a required property.

Sounds good to me, but I checked out this PR #681 is approved, will it be merged? 🤔

unlikely at this point that it will be merged in its current state. in any case, the spec clearly states that name is a required attribute

instead of making name required, we are more likely to change the examples to contain better names. Right now many of them are "default" (my fault) which is not a great name and doesn't really help anyone understand the purpose of the name

I see, will remove default for now and assign better names in examples and tests.

mayurkale22

Lgtm

mayurkale22 · 2020-01-14T01:49:21Z

Please fix the build

…ry-js into xiao/meter-registry

dyladan · 2020-01-14T21:13:53Z

Commit 59fc196 does not have your email associated with it. You must have made it from another computer. In any case the FAQ here at number 7 states that all commits must have your email. That and all subsequent commits on the PR will fail.

mayurkale22 · 2020-01-15T19:25:15Z

Could you please update the PR with respect to #699 (change "Registry" to "Provider")?

dyladan · 2020-01-15T19:37:39Z

Please wait until @bogdandrutu's concerns are addressed. I don't want to have to keep changing this over and over.

…ry#691)

feat: implement named meter

4d2c5fe

xiao-lix requested review from bg451, dyladan, markwolff, mayurkale22, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners January 13, 2020 22:07

Merge branch 'master' into xiao/meter-registry

d5571b9

mayurkale22 reviewed Jan 13, 2020

View reviewed changes

mayurkale22 added enhancement New feature or request size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 13, 2020

xiao-lix added 2 commits January 13, 2020 14:26

chore: address comments

8cecef7

Merge branch 'xiao/meter-registry' of github.com:xiao-lix/opentelemet…

e896f66

…ry-js into xiao/meter-registry

mayurkale22 reviewed Jan 13, 2020

View reviewed changes

chore: remove default name, add names for examples and tests

bfb5010

mayurkale22 approved these changes Jan 14, 2020

View reviewed changes

xiao-lix and others added 4 commits January 14, 2020 10:13

Merge branch 'master' into xiao/meter-registry

1f529a3

chore: fix lint

59fc196

chore: fix linting

2278dd6

Merge branch 'xiao/meter-registry' of github.com:xiao-lix/opentelemet…

702996f

…ry-js into xiao/meter-registry

xiao-lix closed this Jan 15, 2020

xiao-lix deleted the xiao/meter-registry branch January 15, 2020 19:33

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

docs(graphql): corrected optional param in readme usage (open-telemet…

0e8b359

…ry#691)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement named meter #691

feat: implement named meter #691

xiao-lix commented Jan 13, 2020

dyladan commented Jan 13, 2020

mayurkale22 Jan 13, 2020

mayurkale22 Jan 13, 2020 •

edited

Loading

mayurkale22 Jan 13, 2020

codecov-io commented Jan 13, 2020 •

edited

Loading

mayurkale22 Jan 13, 2020

dyladan Jan 13, 2020

xiao-lix Jan 13, 2020

dyladan Jan 13, 2020

dyladan Jan 13, 2020

xiao-lix Jan 13, 2020

mayurkale22 left a comment

mayurkale22 commented Jan 14, 2020

dyladan commented Jan 14, 2020

mayurkale22 commented Jan 15, 2020

dyladan commented Jan 15, 2020

	* This class represents a basic tracer registry which platform libraries can extend
	* This class represents a basic meter registry which platform libraries can extend

feat: implement named meter #691

feat: implement named meter #691

Conversation

xiao-lix commented Jan 13, 2020

Which problem is this PR solving?

Short description of the changes

dyladan commented Jan 13, 2020

Choose a reason for hiding this comment

mayurkale22 Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 13, 2020 • edited Loading

Codecov Report

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

mayurkale22 left a comment

Choose a reason for hiding this comment

mayurkale22 commented Jan 14, 2020

dyladan commented Jan 14, 2020

mayurkale22 commented Jan 15, 2020

dyladan commented Jan 15, 2020

mayurkale22 Jan 13, 2020 •

edited

Loading

codecov-io commented Jan 13, 2020 •

edited

Loading