fix(exporter-collector): default endpoint for node and browser #1197
Conversation
davidwitten
requested review from
bg451,
dyladan,
markwolff,
mayurkale22,
mwear,
naseemkullah,
obecny,
OlivierAlbertini,
rochdev and
vmarchaud
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1197 +/- ##
==========================================
- Coverage 92.17% 92.16% -0.01%
==========================================
Files 120 120
Lines 3410 3409 -1
Branches 697 696 -1
==========================================
- Hits 3143 3142 -1
Misses 267 267
|
| @@ -38,7 +38,7 @@ const { CollectorExporter } = require('@opentelemetry/exporter-collector'); | |||
|
|
|||
| const collectorOptions = { | |||
| serviceName: 'basic-service', | |||
| url: '<opentelemetry-collector-url>' // url is optional and can be omitted - default is http://localhost:55678/v1/trace | |||
| url: '<opentelemetry-collector-url>' // url is optional and can be omitted - default is http://localhost:55678 | |||
There was a problem hiding this comment.
I would mention both (Node and Web) default URLs.
There was a problem hiding this comment.
@mayurkale22 Should I also throw an error/alter the URL when someone enters an incorrect URL? (e.g. entering a URL that ends in /v1/trace for Node)
| @@ -29,6 +29,8 @@ import { toCollectorExportTraceServiceRequest } from '../../transform'; | |||
| import { GRPCQueueItem, TraceServiceClient } from './types'; | |||
| import { removeProtocol } from './util'; | |||
|
|
|||
| const DEFAULT_COLLECTOR_URL = 'http://localhost:55678'; | |||
There was a problem hiding this comment.
I think we should change this URL to localhost:55678 and remove removeProtocol logic from Node specific CollectorExporter module.
There was a problem hiding this comment.
I would change the default url, but would live the removeProtocol to simply make it error proof for users
| @@ -38,6 +40,10 @@ export class CollectorExporter extends CollectorExporterBase< | |||
| window.removeEventListener('unload', this.shutdown); | |||
| } | |||
|
|
|||
| defaultURL(url: string | undefined) { | |||
There was a problem hiding this comment.
I would change the name to describe some action as defaultURL sounds like property and not a method. Maybe setDefaultUrl ?
There was a problem hiding this comment.
For me, getDefaultUrl makes more sense than setDefaultUrl. We are not setting anything as a part of this method. WDYT @obecny @davidwitten ?
There was a problem hiding this comment.
can be getDefaultUrl, chooseDefaultUrl etc. just something that sounds like action / method :)
There was a problem hiding this comment.
@obecny @mayurkale22 Just fixed that, thanks!
|
@obecny would like to get your LGTM on this before merging. |
There is just one tiny thing which I'm missing here, @davidwitten could you please add unit test for that ? |
|
@obecny Just added unit tests! |
Which problem is this PR solving?
This PR addresses issue #1193
Essentially, the same endpoint doesn't work for both Node/browser. Browser requires the endpoint to end in
/v1/traceand doesn't work when it's justlocalhost:55678. Node doesn't work if the endpoint ends in/v1/traceand only works if it's justlocalhost:55678.Short description of the changes
Rather than having one endpoint in the base class (which only worked for browser). I added different default URLs in the node and browser CollectorExporter classes.
Testing
I tested this locally for both node and browser tracing. I also updated the common unit test.