-
Notifications
You must be signed in to change notification settings - Fork 792
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): default endpoint #1070
Conversation
has the url of |
Collector shares the same url between web and node, before grpc the url was fine for both, but now the node has different url the one you changed, whereas the web is still using the old one. This way maybe we should have platform specific default url. Otherwise the new url will not work in web now - so for anyone that uses the web without providing url it won't be working. |
@@ -39,7 +38,7 @@ export function onInit(collectorExporter: CollectorExporter) { | |||
isShutDown: false, | |||
grpcSpansQueue: [], | |||
}); | |||
const serverAddress = removeProtocol(collectorExporter.url); | |||
const serverAddress = collectorExporter.url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of shared url people might put a protocol in front of url and it won't be working - hard to find out why. That's why I added removeProtocol
here to avoid such situation - I would keep it
Hmmm good point. I tried the example with current url and failed to export data to backend via collector in the Node. I think we should use |
|
@mayurkale22 after this PR is merged -> #1063 |
#1063 is merged now, so this can be updated with the platform-specific constants |
Will take care of this sometime today. |
1ea0e17
to
dadcad7
Compare
Not sure how it got closed (I was trying to rebase with master). Anyways, will open new PR. |
This has been tested locally and confirmed to be working.
Current example (collector-exporter-node) works well because we override the url address here.
You would not see traces in backend if you rely on default url (i.e: http://localhost:55678/v1/trace).
/cc @obecny