Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: replace OtlpPipeline with exporter builders #2221
feat: replace OtlpPipeline with exporter builders #2221
Changes from all commits
b836ece
98d3e87
dbdc440
fa1cdb8
dadf93b
f8dd808
43d5857
68c0fb1
12ea48b
563c780
6e27d29
8070e45
d6ab3de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
is it possible for endpoint, protocol to take defaults, so users does not have to specify this (unless they want nondefaults)
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.
good call out, The defaults should already be getting used as they were before, including the
http
appending the appropriate path. I added a couple more test cases for tonic to cover this.I also changed the type of the endpoint field to
Option<String>
so its more explicit that it could be empty, and its on the Builders to handle resolving it.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.
one inconsistency I notice between grpc and http is the end point - one has to append /v1/logs in http, but grpc does not require that. This is something we can address in a follow up.
I am thinking something like:
with_tonic() or with_http() is enough for builder to know which endpoint, protocol to use by default.
builder.with_tonic().build() -- this should just work with defaults
builder.with_http().build() -- this should also just work with defaults.
if user wish to change defaults, then they can use methods like with_endpoint, with_protocol etc.
Lets come back to this after merging this.
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.
sounds good, after my most recent update, the
with_{tonic,http}()
is enough to determine thehttp://localhost:431{7,8}
and the tests for this are updated also.I will however need to look at the spec to see how the
/v1/*
is handled for the two, and I would be happy to create a new pr to get that sorted out.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.
EDIT: After looking back through the code, this should also already be the case, I made some changes to make it a little more clear