-
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
Add Kafka OTEL storage exporter #2135
Conversation
bash -c "set -e; set -o pipefail; $(GOTEST) ./... | $(COLORIZE)" | ||
|
||
.PHONY: test-otel | ||
test-otel: | ||
cd ${OTEL_COLLECTOR_DIR} && bash -c "set -e; set -o pipefail; $(GOTEST) ./... | $(COLORIZE)" |
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.
tests in cmd/opentelemetry-collector
are excluded bc it is a separate go module. I have found only this workaround.
It prints this when go test ./..
is executed from the root dir
go: directory cmd/opentelemetry-collector is outside main module
go: directory cmd/opentelemetry-collector/app/defaults is outside main module
go: directory cmd/opentelemetry-collector/app/exporter is outside main module
go: directory cmd/opentelemetry-collector/app/exporter/kafka is outside main mo
Codecov Report
@@ Coverage Diff @@
## master #2135 +/- ##
==========================================
+ Coverage 96.11% 96.13% +0.02%
==========================================
Files 218 218
Lines 10564 10567 +3
==========================================
+ Hits 10154 10159 +5
Misses 353 353
+ Partials 57 55 -2
Continue to review full report at Codecov.
|
if err != nil { | ||
return nil, err | ||
} | ||
return create(f, config) |
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.
Only this line is not being covered from the whole PR. I cannot pass the initialization, there is no way to change the objects in the Kafka config for mocked builders.
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.
@yurishkuro any idea how to fix the coverage? Even though only a single line is not covered the total is:
ok github.com/jaegertracing/jaeger/cmd/opentelemetry-collector/app/exporter/kafka 0.767s coverage: 94.7% of statements
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 - just some minor things to clarify.
config producer.Configuration | ||
topic string | ||
encoding string | ||
Config producer.Configuration `mapstructure:",squash"` |
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.
Does this need mapstructure:"producer....
?
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 was thinking about it, but given it is scoped under exporters it does not make much sense.
Signed-off-by: Pavol Loffay <[email protected]>
Travis does not want to report the result. hence reopening the PR |
Signed-off-by: Pavol Loffay <[email protected]>
@yurishkuro feel free to comment. I will fix anything in a follow-up PR. |
Resolves jaegertracing/jaeger-opentelemetry-collector#24