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

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

Merged

Conversation

brunoluiz
Copy link
Contributor

@brunoluiz brunoluiz commented Jun 8, 2021

Which problem is this PR solving?

Short description of the changes

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 8, 2021

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jun 9, 2021

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
Copy link
Contributor Author

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

@Flarna
Copy link
Member

Flarna commented Jun 9, 2021

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
Copy link
Contributor Author

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
Copy link
Member

obecny commented Jun 9, 2021

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
Copy link
Member

dyladan commented Jun 16, 2021

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
Copy link
Contributor Author

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.

@@ -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>',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere, this might be warranted

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@brunoluiz
Copy link
Contributor Author

brunoluiz commented Jun 18, 2021

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

Screenshot 2021-06-18 at 10 57 17

@brunoluiz
Copy link
Contributor Author

brunoluiz commented Jun 18, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

dyladan commented Jun 30, 2021

@brunoluiz any update on the browser tests here?

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

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
Copy link
Member

dyladan commented Jun 30, 2021

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 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
@brunoluiz
Copy link
Contributor Author

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.

@dyladan dyladan added the document Documentation-related label Jul 8, 2021
@vmarchaud
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
document Documentation-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants