Add Jaeger exporter factory #136
Conversation
|
Please sign the CLA |
|
I signed it |
Codecov Report
@@ Coverage Diff @@
## master #136 +/- ##
==========================================
- Coverage 69.78% 69.72% -0.06%
==========================================
Files 101 102 +1
Lines 6391 6438 +47
==========================================
+ Hits 4460 4489 +29
- Misses 1696 1709 +13
- Partials 235 240 +5
Continue to review full report at Codecov.
|
| @@ -0,0 +1,12 @@ | |||
| package jaegerexporter | |||
There was a problem hiding this comment.
Make sure all files have the copyright and license notice.
There was a problem hiding this comment.
Can we have this automated? We do have it in the java repo.
| "path" | ||
| "testing" | ||
|
|
||
| "github.com/open-telemetry/opentelemetry-service/config" |
There was a problem hiding this comment.
We normally list imports in the following order:
- Standard library packages.
- 3rd party packages.
- Our own packages.
The 3 sections should be separated by a blank lines. Each section should be sorted alphabetically.
exporter/jaegerexporter/factory.go
Outdated
| if jc.CollectorEndpoint == "" { | ||
| return nil, nil, &jTraceExporterError{ | ||
| code: errCollectorEndpointRequired, | ||
| msg: "Jaeger exporter config requires an Endpoint", |
There was a problem hiding this comment.
Let's refer to config options exactly as they are supposed to be specified in yaml. That will make user's life easier.
| msg: "Jaeger exporter config requires an Endpoint", | |
| msg: "Jaeger exporter config requires an endpoint", |
| // ConfigV2 defines configuration for Jaeger exporter. | ||
| type ConfigV2 struct { | ||
| configmodels.ExporterSettings `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct. | ||
| CollectorEndpoint string `mapstructure:"collector-endpoint,omitempty"` |
There was a problem hiding this comment.
All other exporters use "endpoint" key. Any special reason to use "collector-endpoint" for this one? If not let's use "endpoint" for uniformity.
There was a problem hiding this comment.
I had the same opinion to use "endpoint" until I have found it with the name "collector-endpoint" in "Jaeger.Options", so I wanted to make it explicit.
exporter/jaegerexporter/factory.go
Outdated
| if jc.Username == "" { | ||
| return nil, nil, &jTraceExporterError{ | ||
| code: errUsernameRequired, | ||
| msg: "Jaeger exporter config requires a Username", |
There was a problem hiding this comment.
| msg: "Jaeger exporter config requires a Username", | |
| msg: "Jaeger exporter config requires a username", |
exporter/jaegerexporter/factory.go
Outdated
|
|
||
| exporter, serr := jaeger.NewExporter(jOptions) | ||
| if serr != nil { | ||
| return nil, nil, fmt.Errorf("cannot configure jaeger Trace exporter: %v", serr) |
There was a problem hiding this comment.
| return nil, nil, fmt.Errorf("cannot configure jaeger Trace exporter: %v", serr) | |
| return nil, nil, fmt.Errorf("cannot create Jaeger trace exporter: %v", serr) |
exporter/jaegerexporter/jaeger.go
Outdated
| // errCollectorEndpointRequired indicates that this exporter was not provided with a collector endpoint in its config. | ||
| errCollectorEndpointRequired | ||
|
|
||
| // errUsernameRequired indicates that this exporter was not provided with a user name in its config. |
There was a problem hiding this comment.
| // errUsernameRequired indicates that this exporter was not provided with a user name in its config. | |
| // errUsernameRequired indicates that this exporter was not provided with a username in its config. |
exporter/jaegerexporter/factory.go
Outdated
| return nil, nil, err | ||
| } | ||
|
|
||
| return jexp, noopStopFunc, nil |
There was a problem hiding this comment.
Stop function is needed for Jaeger. It should not be noop. It needs to call exporter.Flush(). See current implementation in jaeger.go/JaegerExportersFromViper.
There was a problem hiding this comment.
I attempted the same implementation je.stop in a previous commit following "exporter.Flush()" but I thought maybe I would try to do nothing and It will be reviewed anyway.
|
Hi @MohamedElqdusy thanks for your PR! We do appreciate it! However, I don't think that we should be enabling OC exporters from client libraries at this stage of OTel Service project: the reason is that those exporters typically have a few issues that we plan to address on our roadmap to OTel service. For now the only exporter from OC client libraries that we should use is the OC agent one. That said we need to rectify the situation rather soon so people can start to experiment with the service. Sorry for not making that more visible and having you going to the trouble of creating this PR. |
|
Hi @pjanotti Thank you for letting me know, It's understood, I enjoyed my time anyway. |
|
Thanks for understanding @MohamedElqdusy |
|
Closing PR per conversation. |
#36