[wip] add support for console/otlp metric exporter #7644
Conversation
This is a prototype for adding support for metric readers to service.telemetry.metrics. The parsing code was generated using the json schema generator go-jsonschema, with the source schema in this PR: open-telemetry/opentelemetry-configuration#15 There are many open questions still. For this to move forward, i propose it should be behind a separate feature gate to allow users who want to experiment with it to do so. Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
codeboten
force-pushed
the
codeboten/add-otlp-metric-exporter
branch
from
84767ea
to
27f1b28
Compare
Signed-off-by: Alex Boten <aboten@lightstep.com>
| if len(cfg.Metrics.Address) > 0 { | ||
| settings.Logger.Warn("service.telemetry.metrics.address is being deprecated in favor of service.telemetry.metrics.metric_readers") |
There was a problem hiding this comment.
Do we need to somehow feature gate this behavior where the user has enabled the UseOTel feature gate but hasn't started using the Reader config yet? Sounds like we eventually we want users to configure the Readers and won't support address, but that would be a breaking change.
There was a problem hiding this comment.
Yup, I'm planning on putting this all behind this feature gate: #7678
| case "certificate": | ||
| case "client_key": | ||
| case "client_certificate": |
There was a problem hiding this comment.
Do we need these cases if we don't do anything with them?
There was a problem hiding this comment.
These were here as a reminder we need to do something with them :) once i split this PR into smaller ones, i'll fill in these details
| func newOTLPGRPCMetricExporter(ctx context.Context, args map[string]interface{}) (sdkmetric.Exporter, error) { | ||
| opts := []otlpmetricgrpc.Option{} | ||
| for k, v := range args { | ||
| switch k { |
There was a problem hiding this comment.
IDK what the configuration file spec looks like, but if a user configures an unsupported value should we error?
There was a problem hiding this comment.
i think it would make sense for an error to be returned for unsupported values
| func toStringMap(in map[string]interface{}) map[string]string { | ||
| out := map[string]string{} | ||
| for k, v := range in { | ||
| out[k] = fmt.Sprintf("%v", v) |
There was a problem hiding this comment.
Would it be easier to call fmt.Sprint instead?
| case "client_key": | ||
| case "client_certificate": | ||
| case "compression": | ||
| switch fmt.Sprintf("%s", v) { |
There was a problem hiding this comment.
At this point you can't assert that v is a string due to args being a map of string to any, switching to fmt.Sprint should work without issue here.
| case "timeout": | ||
| timeout, ok := v.(int) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid timeout for otlp exporter: %s", v) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("invalid timeout for otlp exporter: %s", v) | |
| return nil, fmt.Errorf("invalid timeout for otlp exporter: %v", v) |
| _, err := tel.initPrometheus(res, settings.Logger, cfg.Metrics.Address, cfg.Metrics.Level, asyncErrorChannel) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Should this return in general instead of just if an error occurred?
This adds the feature-gate telemetry.useOtelWithExtendedConfigurationForInternalTelemetry to start adding support for additional exporters for the collector's internal telemetry. This is part of of #7644 --------- Signed-off-by: Alex Boten <aboten@lightstep.com>
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This is a prototype for adding support for metric readers to service.telemetry.metrics. The parsing code was generated using the json schema generator go-jsonschema, with the source schema in this PR: open-telemetry/opentelemetry-configuration#15
There are many open questions still. For this to move forward, i propose it should be behind a separate feature gate to allow users who want to experiment with it to do so.
Fix #7641