fix(exporter-collector-grpc): incorrect URL format on docs after 0.20.0 update #2266

brunoluiz · 2021-06-08T23:40:58Z

Which problem is this PR solving?

Fixes the connection issue mentioned at @opentelemetry/exporter-collector-grpc not connecting after 0.20.0 upgrade #2264, where exporter-collector-grpc is not connecting to otel grpc connector when using localhost:4317. This was partially due to an undocumented URL format, changed after 0.20.0

Short description of the changes

Changes documentation from localhost:4317 to grpc://localhost:4317 for @opentelemetry/exporter-collector-grpc package

linux-foundation-easycla · 2021-06-08T23:41:02Z

The committers are authorized under a signed CLA.

✅ Bruno Luiz Silva (99bffbe)

codecov · 2021-06-09T09:43:17Z

Codecov Report

Merging #2266 (a820b64) into main (611a30f) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2266      +/-   ##
==========================================
+ Coverage   92.75%   92.77%   +0.01%     
==========================================
  Files         145      145              
  Lines        5216     5216              
  Branches     1068     1068              
==========================================
+ Hits         4838     4839       +1     
+ Misses        378      377       -1

Impacted Files	Coverage Δ
...emetry-api-metrics/src/platform/node/globalThis.ts	`100.00% <0.00%> (+100.00%)`	⬆️

brunoluiz · 2021-06-09T10:30:19Z

I am a bit lost with those CI errors... I suppose these issues were not due to my changes or were they?

Flarna · 2021-06-09T10:37:44Z

Looking into the asserts which fail it looks very much like it is because of the changes here

      Uncaught AssertionError [ERR_ASSERTION]: 'grpc://localhost:4317' === 'localhost:4317'
      + expected - actual

brunoluiz · 2021-06-09T15:10:37Z

Seems the NodeJS 16 CI test failed. Is there a way which I can re-trigger it or is it fine as it is?

obecny · 2021-06-09T21:40:06Z

Can you please ensure this is working fine with this example

You can also update the docker compose to the latest version you were testing.

If all is fine we tend to update readme with information about last tested version so that it is clear which version is compatible with this as spec and collector were not always in sync.

To boostrap all you can modify temporary lerna.json and add new entry to packages section this way you will be able to debug and use the local version of the remaining packages

  "packages": [
    "examples/collector-exporter-node",

dyladan · 2021-06-16T13:20:42Z

Seems the NodeJS 16 CI test failed. Is there a way which I can re-trigger it or is it fine as it is?

The node 16 test is fine, but the browser test failure is probably real.

brunoluiz · 2021-06-17T11:46:22Z

I will try to run re-run the tests in composer during the weekend. The browser test is probably something that I missed while searching and replacing the default URL updates.

spencerwilson · 2021-06-18T01:54:10Z

packages/opentelemetry-exporter-collector-grpc/README.md

@@ -27,7 +27,7 @@ const { BasicTracerProvider, SimpleSpanProcessor } = require('@opentelemetry/tra
 const { CollectorTraceExporter } =  require('@opentelemetry/exporter-collector-grpc');

 const collectorOptions = {
-  // url is optional and can be omitted - default is localhost:4317
+  // url is optional and can be omitted - default is grpc://localhost:4317
  url: '<collector-hostname>:<port>',


Here and elsewhere, this might be warranted

Suggested change

url: '<collector-hostname>:<port>',

url: '<collector-scheme>://<collector-hostname>:<port>',

But, for this specific case, where the collector is grpc only, shouldn't we inform the use the required scheme is grpc? Or does it actually support other schemes?

then it should be 'grpc://<collector-hostname>:<port>' right ?

brunoluiz · 2021-06-18T09:58:53Z

So, just tested it based on the examples and it seems fine. I need to note though that, on Zipkin, the service is marked as unknown instead of BASIC-SERVICE. I need to understand how to check the browser issue.

EDIT: Just realised that by default that example doesn't use the grpc collector, so I am re-testing it

brunoluiz · 2021-06-18T10:01:21Z

If all is fine we tend to update readme with information about last tested version so that it is clear which version is compatible with this as spec and collector were not always in sync.

I tested with the version which is in the README currently, so should be all good 😉

brunoluiz · 2021-06-18T10:08:34Z

I just re-ran the browsers tests locally through lerna run --scope @opentelemetry/instrumentation-fetch test:browser and it seems to work. Could someone re-trigger the browser test in the CI just to see if it is fine?

brunoluiz · 2021-06-18T10:55:43Z

Ok, so something is off here. With the example, it works as far as I add the config.url=grpc://localhost:4317. But, if left empty and leave it to go with the default config, this will error.

Checking a bit the code, I found this: https://github.com/open-telemetry/opentelemetry-js/blame/main/packages/opentelemetry-exporter-collector-grpc/src/util.ts#L110-L122

It seems it is just used to check if the schema is either HTTP or GRPC, but actually it doesn't do anything with it, as it returns target.host at the end. The default value I added in this PR contains grpc://, which obviously breaks the GRPC client later on.

Should I revert the default values and just add documentation that grpc:// or http:// is required on the url?

Another thing as well: the grpc-collector is not sending data to the Zipkin, but I guess this is what I mentioned on issue #2264.

vmarchaud · 2021-06-20T16:14:56Z

Checking a bit the code, I found this: main/packages/opentelemetry-exporter-collector-grpc/src/util.ts#L110-L122 (blame)

I find it odd that the collector grpc check for https since it only support grpc.

Should I revert the default values and just add documentation that grpc:// or http:// is required on the url?

I think that would be the way to go if grpc itself doesn't support using just a hostname/port combination

dyladan · 2021-06-30T16:08:34Z

@brunoluiz any update on the browser tests here?

brunoluiz · 2021-06-30T17:19:05Z

As mentioned on #2266 (comment), I've reverted the changes and just kept the README changes. The utils might have some reason to exist, but changing default values causes it to crash. I suppose more investigation might be required?

dyladan · 2021-06-30T18:51:41Z

In the PR title you have incorrect default URL. Is the default URL actually bad or is it just the documented example with the bad URL?

brunoluiz · 2021-07-08T12:07:10Z

In the PR title you have incorrect default URL. Is the default URL actually bad or is it just the documented example with the bad URL?
After discussion here, I reverted the changes on the default URL itself, as the default URLs work fine. But, if the developer specifies some other URL, it requires the grpc scheme to be defined.

Due to that, I just changed title and description accordingly. Let me know if it ready to merge.

vmarchaud · 2021-07-11T07:55:11Z

Another PR has been opened to update the behavior to automatically add grpc:// if not set, but i think its make sense to tell users that its required anyway in the docs

fix(exporter-collector-grpc): incorrect default URL after 0.20.0 update

99bffbe

brunoluiz requested review from dyladan, Flarna, johnbley, legendecas, markwolff, mayurkale22, MSNev, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners June 8, 2021 23:40

fix collector metric exporter test assertion

db63421

dyladan approved these changes Jun 9, 2021

View reviewed changes

MSNev approved these changes Jun 17, 2021

View reviewed changes

spencerwilson reviewed Jun 18, 2021

View reviewed changes

Merge branch 'open-telemetry:main' into fix-default-grpc-collector-url

8db1693

brunoluiz added 2 commits June 30, 2021 18:04

chore: add changes to README and revert other changes

3aa9ff3

Merge branch 'main' into fix-default-grpc-collector-url

a1e63de

brunoluiz force-pushed the fix-default-grpc-collector-url branch from c52ad38 to a1e63de Compare June 30, 2021 17:06

brunoluiz changed the title ~~fix(exporter-collector-grpc): incorrect default URL after 0.20.0 update~~ fix(exporter-collector-grpc): incorrect URL format on docs after 0.20.0 update Jul 8, 2021

Merge branch 'main' into fix-default-grpc-collector-url

a820b64

dyladan added the document Documentation-related label Jul 8, 2021

vmarchaud approved these changes Jul 11, 2021

View reviewed changes

vmarchaud merged commit 8ed5cd1 into open-telemetry:main Jul 11, 2021

dyladan mentioned this pull request Jul 26, 2021

chore: 0.24.0 release proposal #2377

Merged

snyk-bot mentioned this pull request Feb 22, 2022

[Snyk] Upgrade @opentelemetry/exporter-collector from 0.4.0 to 0.24.0 dfds/micro-frontend-poc#10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(exporter-collector-grpc): incorrect URL format on docs after 0.20.0 update #2266

fix(exporter-collector-grpc): incorrect URL format on docs after 0.20.0 update #2266

brunoluiz commented Jun 8, 2021 •

edited

linux-foundation-easycla bot commented Jun 8, 2021 •

edited

codecov bot commented Jun 9, 2021 •

edited

brunoluiz commented Jun 9, 2021

Flarna commented Jun 9, 2021

brunoluiz commented Jun 9, 2021

obecny commented Jun 9, 2021 •

edited

dyladan commented Jun 16, 2021

brunoluiz commented Jun 17, 2021

spencerwilson Jun 18, 2021

brunoluiz Jun 18, 2021

vmarchaud Jun 20, 2021

brunoluiz commented Jun 18, 2021 •

edited

brunoluiz commented Jun 18, 2021 •

edited

brunoluiz commented Jun 18, 2021

brunoluiz commented Jun 18, 2021

vmarchaud commented Jun 20, 2021

dyladan commented Jun 30, 2021

brunoluiz commented Jun 30, 2021

dyladan commented Jun 30, 2021

brunoluiz commented Jul 8, 2021

vmarchaud commented Jul 11, 2021

	url: '<collector-hostname>:<port>',
	url: '<collector-scheme>://<collector-hostname>:<port>',

fix(exporter-collector-grpc): incorrect URL format on docs after 0.20.0 update #2266

fix(exporter-collector-grpc): incorrect URL format on docs after 0.20.0 update #2266

Conversation

brunoluiz commented Jun 8, 2021 • edited

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented Jun 8, 2021 • edited

codecov bot commented Jun 9, 2021 • edited

Codecov Report

brunoluiz commented Jun 9, 2021

Flarna commented Jun 9, 2021

brunoluiz commented Jun 9, 2021

obecny commented Jun 9, 2021 • edited

dyladan commented Jun 16, 2021

brunoluiz commented Jun 17, 2021

spencerwilson Jun 18, 2021

Choose a reason for hiding this comment

brunoluiz Jun 18, 2021

Choose a reason for hiding this comment

vmarchaud Jun 20, 2021

Choose a reason for hiding this comment

brunoluiz commented Jun 18, 2021 • edited

brunoluiz commented Jun 18, 2021 • edited

brunoluiz commented Jun 18, 2021

brunoluiz commented Jun 18, 2021

vmarchaud commented Jun 20, 2021

dyladan commented Jun 30, 2021

brunoluiz commented Jun 30, 2021

dyladan commented Jun 30, 2021

brunoluiz commented Jul 8, 2021

vmarchaud commented Jul 11, 2021

brunoluiz commented Jun 8, 2021 •

edited

linux-foundation-easycla bot commented Jun 8, 2021 •

edited

codecov bot commented Jun 9, 2021 •

edited

obecny commented Jun 9, 2021 •

edited

brunoluiz commented Jun 18, 2021 •

edited

brunoluiz commented Jun 18, 2021 •

edited