Update to Proto v0.5.0 #1588

obecny · 2020-10-09T17:22:12Z

Which problem is this PR solving?

Fixes update proto ver. 0.5.0 and collector #1574

Short description of the changes

Upgrading to proto ver. 0.5.0
Upgrading example to use collector ver. 0.12
Upgrading metrics to reflects latest changes in spec as well as in proto ver. 0.5.0
Updating Prometheus

…metrics to latest spec changes

codecov · 2020-10-09T17:27:08Z

Codecov Report

Merging #1588 into master will decrease coverage by 0.04%.
The diff coverage is 85.96%.

@@            Coverage Diff             @@
##           master    #1588      +/-   ##
==========================================
- Coverage   91.44%   91.40%   -0.05%     
==========================================
  Files         165      165              
  Lines        5052     5013      -39     
  Branches     1034     1023      -11     
==========================================
- Hits         4620     4582      -38     
+ Misses        432      431       -1

Impacted Files	Coverage Δ
packages/opentelemetry-api/src/metrics/Metric.ts	`100.00% <ø> (ø)`
...ry-exporter-prometheus/src/PrometheusSerializer.ts	`97.84% <ø> (+1.62%)`	⬆️
...emetry-metrics/src/export/ConsoleMetricExporter.ts	`62.50% <0.00%> (+3.67%)`	⬆️
...ckages/opentelemetry-metrics/src/export/Batcher.ts	`92.00% <75.00%> (+0.69%)`	⬆️
...lemetry-exporter-collector/src/transformMetrics.ts	`92.59% <82.35%> (-4.19%)`	⬇️
.../opentelemetry-exporter-collector/src/transform.ts	`87.77% <100.00%> (+0.96%)`	⬆️
...ages/opentelemetry-exporter-collector/src/types.ts	`100.00% <100.00%> (ø)`
...emetry-metrics/src/export/aggregators/LastValue.ts	`100.00% <100.00%> (ø)`
...ntelemetry-metrics/src/export/aggregators/index.ts	`100.00% <100.00%> (ø)`
packages/opentelemetry-metrics/src/export/types.ts	`100.00% <100.00%> (ø)`

…proto_v050

examples/collector-exporter-node/package.json

obecny · 2020-10-13T17:13:52Z

@vmarchaud I know you have tested all 3 versions previously, would you be able to test it again, so that one more person will check if all is working fine, just to double check I haven't missed anything ?

vmarchaud · 2020-10-13T19:50:47Z

@obecny sure, i will test tomorrow and update if its fine for me

vmarchaud · 2020-10-14T14:21:26Z

Got another error:

{"stack":"TypeError: Cannot read property 'counts' of undefined\n    at toHistogramPoint (/home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/transformMetrics.js:89:37)\n    at toCollectorMetric (/home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/transformMetrics.js:138:34)\n    at /home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/transformMetrics.js:201:40\n    at Array.map (<anonymous>)\n    at toCollectorInstrumentationLibraryMetrics (/home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/transformMetrics.js:201:26)\n    at /home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/transformMetrics.js:214:106\n    at Function.from (<anonymous>)\n    at /home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/transformMetrics.js:214:50\n    at Function.from (<anonymous>)\n    at toCollectorResourceMetrics (/home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/transformMetrics.js:211:18)\n    at Object.toCollectorExportMetricServiceRequest (/home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/transformMetrics.js:165:26)\n    at CollectorMetricExporter.convert (/home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/platform/node/CollectorMetricExporter.js:33:35)\n    at CollectorMetricExporter.send (/home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/platform/node/CollectorExporterNodeBase.js:43:37)\n    at /home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/CollectorExporterBase.js:70:22\n    at new Promise (<anonymous>)\n    at CollectorMetricExporter._export (/home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/CollectorExporterBase.js:67:16)\n    at CollectorMetricExporter.export (/home/vmarchaud/opentelemetry/obecny-fork/packages/opentelemetry-exporter-collector/build/src/CollectorExporterBase.js:52:14)\n    at /home/vmarchaud/reelevant/node-utils-monorepo/packages/telemetry/node_modules/@opentelemetry/metrics/build/src/export/Controller.js:42:34\n    at new Promise (<anonymous>)\n    at PushController._collect (/home/vmarchaud/reelevant/node-utils-monorepo/packages/telemetry/node_modules/@opentelemetry/metrics/build/src/export/Controller.js:41:16)","message":"Cannot read property 'counts' of undefined","name":"TypeError"}

obecny · 2020-10-14T16:13:55Z

transformMetrics.js:89:37

did you bootstrap all, especially metrics - there were changes

vmarchaud · 2020-10-14T16:18:17Z

did you bootstrap all, especially metrics - there were changes

Yes i did, the repo is a fresh git clone of your branch with lerna bootstrap

EDIT: I dont think we use histogram (only value recorder) in our app too, so i dont understand why i can get the error

obecny · 2020-10-14T16:19:16Z

did you bootstrap all, especially metrics - there were changes

Yes i did, the repo is a fresh git clone of your branch with lerna bootstrap

ok so just pls tell me how can I reproduce this error

obecny · 2020-10-14T16:20:20Z

EDIT: I dont think we use histogram (only value recorder) in our app too, so i dont understand why i can get the error

Value Recorder has now Histogram aggregator

vmarchaud · 2020-10-14T16:27:32Z

ok so just pls tell me how can I reproduce this error

Well i'm just creating a new instrument and recording a value, i'm looking at the implementation to see why this error could happen

obecny · 2020-10-14T16:39:54Z

ok so just pls tell me how can I reproduce this error

Well i'm just creating a new instrument and recording a value, i'm looking at the implementation to see why this error could happen

ok I have just modified the example examples/collector-exporter-node/metrics.js

const recorder = meter.createValueRecorder('test_recorder', {
  description: 'Example of a ValueRecorder',
});

const labels = { pid: process.pid, environment: 'staging' };

setInterval(() => {
  requestCounter.bind(labels).add(1);
  upDownCounter.bind(labels).add(Math.random() > 0.5 ? 1 : -1);
  recorder.bind(labels).update(Math.random());
}, 1000);

Running docker from this example
All works fine, can see traces on http://localhost:9090/graph?g0.range_input=1m&g0.expr=test_recorder_sum&g0.tab=0

vmarchaud · 2020-10-14T16:51:04Z

Isn't the method used to record a value for the ValueRecorder record and not update as in your example ?
Anyway i think its linked to the fact that the API version is different as the sdk one. I think its safe to merge this

obecny · 2020-10-14T16:58:06Z

Isn't the method used to record a value for the ValueRecorder record and not update as in your example ?

You right it is record - although

  record(value: number): void {
    this.update(value);
  }

Anyway i think its linked to the fact that the API version is different as the sdk one. I think its safe to merge this

Ok, but I'm wondering why are you getting this error then, the api and metrics changed, Did you use a different example with possibility of different api or ?

…proto_v050

obecny · 2020-10-14T18:10:32Z

@dyladan any idea why EasyCLA is in "waiting mode" ?

dyladan · 2020-10-14T18:11:06Z

This is a known bug. Have to close and reopen the PR to retrigger the CLA check.

vmarchaud · 2020-10-14T18:12:19Z

Ok, but I'm wondering why are you getting this error then, the api and metrics changed, Did you use a different example with possibility of different api or ?

I didn't follow any example, just implemented the API from my understand. I will try to debug it further tomorrow

obecny · 2020-10-15T12:26:38Z

Ok, but I'm wondering why are you getting this error then, the api and metrics changed, Did you use a different example with possibility of different api or ?

I didn't follow any example, just implemented the API from my understand. I will try to debug it further tomorrow

We should use the same example for testing and verifying - by default this is examples/collector-exporter-node if this needs to be changed or updated to cover some additional scenario then we should do it, otherwise it is hard to verify what is not working and why :/

vmarchaud · 2020-10-15T14:39:03Z

We should use the same example for testing and verifying - by default this is examples/collector-exporter-node if this needs to be changed or updated to cover some additional scenario then we should do it, otherwise it is hard to verify what is not working and why :/

Well the example is working for me too, i generally prefer to test with a whole service to avoid bad surprise.
Also i know why it wasnt working: i didn't use the correct metrics sdk. It's working fine now.

obecny · 2020-10-15T17:05:04Z

We should use the same example for testing and verifying - by default this is examples/collector-exporter-node if this needs to be changed or updated to cover some additional scenario then we should do it, otherwise it is hard to verify what is not working and why :/

Well the example is working for me too, i generally prefer to test with a whole service to avoid bad surprise.
Also i know why it wasnt working: i didn't use the correct metrics sdk. It's working fine now.

OK I'm just waiting then for your approval

vmarchaud · 2020-10-16T13:40:24Z

@obecny Sorry i forgot the re-approve !

PixnBits · 2021-05-05T19:40:45Z

packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts

-                  '# TYPE value_recorder summary',
+                  '# TYPE value_recorder histogram',


it('should export a ValueRecorder as a summary'

but this assertion was changed to a histogram, is the assertion needed until summaries are brought back? (ex: #982) probably dependent on specification updates?
(would opening a PR to remove this test be appropriate?)

* chore: updating submodule for opentelemetry-proto * chore: necessary changes after upgrade to proto ver. 0.5.0, aligning metrics to latest spec changes * chore: removing examples from lerna bootstrap * chore: cleaning ups unused interfaces * chore: fixing unit test * chore: updating aggregation temporality rules * chore: updating temporality for value recorder * chore: removing unneeded lib * chore: span id and trace id as hex * chore: adding value recorder to example

obecny added 2 commits October 1, 2020 17:27

chore: updating submodule for opentelemetry-proto

f67f031

chore: necessary changes after upgrade to proto ver. 0.5.0, aligning …

2325595

…metrics to latest spec changes

obecny requested review from dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, OlivierAlbertini and vmarchaud as code owners October 9, 2020 17:22

obecny self-assigned this Oct 9, 2020

Merge branch 'master' into proto_v050

27319df

obecny mentioned this pull request Oct 9, 2020

chore: do not convert ids to base64 #1583

Closed

obecny added 4 commits October 9, 2020 19:28

chore: removing examples from lerna bootstrap

1fc672f

Merge branch 'proto_v050' of github.com:obecny/opentelemetry-js into …

4a6250d

…proto_v050

chore: cleaning ups unused interfaces

440bf39

chore: fixing unit test

4752a0e

obecny added the enhancement New feature or request label Oct 9, 2020

obecny mentioned this pull request Oct 9, 2020

Collector-exporter-node example is not working, no metrics are exposed #1587

Closed

vmarchaud approved these changes Oct 10, 2020

View reviewed changes

vmarchaud added the breaking label Oct 10, 2020

chore: updating aggregation temporality rules

d41cd87

dyladan approved these changes Oct 13, 2020

View reviewed changes

examples/collector-exporter-node/package.json Outdated Show resolved Hide resolved

obecny added 3 commits October 13, 2020 22:08

chore: updating temporality for value recorder

1175b41

Merge branch 'master' into proto_v050

3bb4ee3

chore: removing unneeded lib

a088308

mwear approved these changes Oct 13, 2020

View reviewed changes

obecny added 3 commits October 14, 2020 19:03

Merge branch 'master' into proto_v050

4543a37

chore: adding value recorder to example

7d333db

Merge branch 'proto_v050' of github.com:obecny/opentelemetry-js into …

3bcab3b

…proto_v050

dyladan closed this Oct 14, 2020

dyladan reopened this Oct 14, 2020

vmarchaud approved these changes Oct 16, 2020

View reviewed changes

dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Oct 16, 2020

obecny merged commit 00a8ce7 into open-telemetry:master Oct 19, 2020

obecny deleted the proto_v050 branch October 19, 2020 11:37

dyladan mentioned this pull request Oct 26, 2020

exporter-collector-grpc traceId and spanId values in protobuf are incorrect #1623

Closed

dyladan linked an issue Oct 26, 2020 that may be closed by this pull request

Use hex encoding for trace id and span id fields in OTLP JSON encoding #1513

Closed

PixnBits reviewed May 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to Proto v0.5.0 #1588

Update to Proto v0.5.0 #1588

obecny commented Oct 9, 2020

codecov bot commented Oct 9, 2020 •

edited

obecny commented Oct 13, 2020

vmarchaud commented Oct 13, 2020

vmarchaud commented Oct 14, 2020 •

edited

obecny commented Oct 14, 2020 •

edited

vmarchaud commented Oct 14, 2020 •

edited

obecny commented Oct 14, 2020

obecny commented Oct 14, 2020 •

edited

vmarchaud commented Oct 14, 2020

obecny commented Oct 14, 2020

vmarchaud commented Oct 14, 2020

obecny commented Oct 14, 2020 •

edited

obecny commented Oct 14, 2020

dyladan commented Oct 14, 2020

vmarchaud commented Oct 14, 2020

obecny commented Oct 15, 2020

vmarchaud commented Oct 15, 2020

obecny commented Oct 15, 2020

vmarchaud commented Oct 16, 2020

PixnBits May 5, 2021 •

edited

		'# TYPE value_recorder summary',
		'# TYPE value_recorder histogram',

Update to Proto v0.5.0 #1588

Update to Proto v0.5.0 #1588

Conversation

obecny commented Oct 9, 2020

Which problem is this PR solving?

Short description of the changes

codecov bot commented Oct 9, 2020 • edited

Codecov Report

obecny commented Oct 13, 2020

vmarchaud commented Oct 13, 2020

vmarchaud commented Oct 14, 2020 • edited

obecny commented Oct 14, 2020 • edited

vmarchaud commented Oct 14, 2020 • edited

obecny commented Oct 14, 2020

obecny commented Oct 14, 2020 • edited

vmarchaud commented Oct 14, 2020

obecny commented Oct 14, 2020

vmarchaud commented Oct 14, 2020

obecny commented Oct 14, 2020 • edited

obecny commented Oct 14, 2020

dyladan commented Oct 14, 2020

vmarchaud commented Oct 14, 2020

obecny commented Oct 15, 2020

vmarchaud commented Oct 15, 2020

obecny commented Oct 15, 2020

vmarchaud commented Oct 16, 2020

PixnBits May 5, 2021 • edited

Choose a reason for hiding this comment

codecov bot commented Oct 9, 2020 •

edited

vmarchaud commented Oct 14, 2020 •

edited

obecny commented Oct 14, 2020 •

edited

vmarchaud commented Oct 14, 2020 •

edited

obecny commented Oct 14, 2020 •

edited

obecny commented Oct 14, 2020 •

edited

PixnBits May 5, 2021 •

edited