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): default endpoint #1070

Closed

Conversation

mayurkale22
Copy link
Member

/cc @obecny

@mayurkale22 mayurkale22 added the bug Something isn't working label May 18, 2020
@obecny
Copy link
Member

obecny commented May 18, 2020

has the url of opentelemetry-collector endpoint changed ?

@obecny
Copy link
Member

obecny commented May 18, 2020

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

@obecny obecny May 18, 2020

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

@mayurkale22
Copy link
Member Author

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.

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 http://localhost:55678/v1/trace for Web and localhost:55678 for Node. This is what we have done in OpenCensus. Any thoughts?

@obecny
Copy link
Member

obecny commented May 18, 2020

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.

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 http://localhost:55678/v1/trace for Web and localhost:55678 for Node. This is what we have done in OpenCensus. Any thoughts?

constants.ts in platform folder with DEFAULT_URL ?

@obecny
Copy link
Member

obecny commented May 19, 2020

@mayurkale22 after this PR is merged -> #1063
changing the url to be platform dependent should be easier

@dyladan
Copy link
Member

dyladan commented Jun 3, 2020

#1063 is merged now, so this can be updated with the platform-specific constants

@mayurkale22
Copy link
Member Author

#1063 is merged now, so this can be updated with the platform-specific constants

Will take care of this sometime today.

@mayurkale22
Copy link
Member Author

Not sure how it got closed (I was trying to rebase with master). Anyways, will open new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants