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
Init Elasticsearch exporter #2324
Init Elasticsearch exporter #2324
Changes from 1 commit
66f7074
04f1394
98258d5
cd186b1
9044bc2
bf95ba3
5ce6181
5337d63
63575c0
758b2c2
13243f6
b4d147b
617048e
9255024
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.
How does a deployment typically look like? Is it common to have TLS for one server, but not for another? Do they have different sets of certs, or would a client only need one CA that works for all clients? I'm asking because endpoints are typically only
host:port
, without the protocol part, using TLS by default.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.
No, when using TLS one would want to have TLS for the complete cluster.
Ideally one CA cert.
The exporter also accepts
localhost:9200
, but I'm not sure if TLS will be automatically enabled or disabled by default. I assume the client library will expand this tohttp://localhost:9200
if tls is not explicitely configured.I took otlphttpexporter as example, which also uses a full URL in its example:
same for zipkin.
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.
@bogdandrutu ^
Interesting, the one from OTLP should have been named URL then, instead of Endpoint. Looks good to me then!
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.
Consider "num_workers" to be consistent with NumConsumers in our sending_queue helper.
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.
+1
Done
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.
MaxRequests
? If I were to add this to https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/exporterhelper/queued_retry.go#L58 that would probably be a more consistent name.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.
+1
Updated field name and configuration name to
max_requests
.