-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[exporter/Logzioexporter] logs support #10821
[exporter/Logzioexporter] logs support #10821
Conversation
Hi, @dashpole thanks for replying. |
You might want to consider gating the configuration change with a feature-gate. I did this in the googlecloud exporter when we made changes to our config. See https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/googlecloudexporter/factory.go#L55 for an example of using different configurations based on a feature-gate. |
On a separate note, I don't see a CODEOWNERS entry for this component. It seems like you and potentially @jkowall or @Doron-Bargo (who were cc'd on the status header PR) would be reasonable owners? |
Thanks for the advice @dashpole, I'll take a look at that :) |
yes, please add owners in a separate PR. If you could have one of the other codeowners review this PR, that would be helpful. |
I checked and looks like this component does have an entry in CODEOWNERS: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/.github/CODEOWNERS#L45 |
Ah, sorry. I searched for logsio... |
Hi @dashpole :) |
You wouldn't need to feature-gate the addition of logs support -- just the removal of config options from your stable configuration. You just have to abide by this, since your component is stable:
A feature-gate, which is initially disabled by default, is one way to provide prior notice of removal of those options, since users can temporarily (during beta) re-enable them. But if you have a different way you'd like to provide prior notice (e.g. a warning log when using one of the options you plan to remove), that would also be acceptable. You should have at least one release where it is in alpha and one in beta, after which you can remove the feature gate. |
Ok, that makes sense. |
…d to warn for deprecation + unit test
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.
I've reviewed the config + readme changes, which look good. I'll defer to one of the codeowners (who are hopefully more familiar with logzio) to review the log exporter implementation.
I can't, since they don't appear to be org members. Its fine though; they can still review. |
…arnDeprecatedOptions
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.
lgtm
Hi @dashpole. |
…o logzioexporter-logs-support # Conflicts: # exporter/logzioexporter/go.mod # exporter/logzioexporter/go.sum
@yotamloe can you fix the merge conflicts? |
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.
This PR adds some new public API, is it intentional? In general, functions, methods and structs should be made private so that we can change the internals of the exporter without making breaking changes.
@mx-psi 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.
Thanks!! Just a couple more that I missed and we can merge this :)
Description:
logzioexporter update:
logzio-go
andjaeger-logzio
dependencieslogzioSpan
,logzioService
,jsonLog
in object directiryexport
method for traces and logs using HTTP clientcustom_endpoint
drain_interval
queue_capacity
queue_max_length
Testing:
Added tests for logs pipeline and for logzio objects in the following files:
exporter_test.go
config_test.go
factory_test.go
jsonlog_test.go
logziospan_test.go
Passing all unit tests:
coverage: 82% of statements
Documentation:
exporterhelper
configuration option