✨🔧 Setup Open Telemetry Metrics and Traces #237
Conversation
Closes EinStack#221 We use `go.opentelemetry.io/contrib/exporters/autoexport`` for standard loading exporter configurations via env variables. This commit, initialize metrics meter and tracer. There is no need to pass the providers in `*telemetry.Telemetry` because they are globally available via `otel` package. Only setup is implemented here to simplify review. Instrumentation will be done in followup patches.
roma-glushko
changed the title
roma-glushko
added
type:enhancement
|
|
||
| var Resource = resource.NewWithAttributes( | ||
| semconv.SchemaURL, | ||
| semconv.ServiceName("glide"), |
There was a problem hiding this comment.
We also should pass the service version and service.instance.id. Plus, it's nice to be able to override those values like OTEL collector allows (https://github.com/open-telemetry/opentelemetry-collector/blob/4d73b4f1bfee7e710521d29b094c2c2ce473b2c7/service/internal/resource/config.go#L16-L15)
There was a problem hiding this comment.
sure , that is beyond the scope of the PR. This is strictly for setting up providers. Your comment falls under instrumentation.
There was a problem hiding this comment.
That's fine to leave it out in this PR but #221 was including that thing explicitly. Dont' want us to miss that requirement along the way if we close the task
There was a problem hiding this comment.
My bad! I thought about discussing it there but got sidetracked. @roma-glushko is there a way to squeeze this in existing instrumentation tasks ?
There was a problem hiding this comment.
Sure thing, no worries! I will move it into a sep task
| sdktrace.WithBatcher(spanExporter), | ||
| ) | ||
| otel.SetTracerProvider(tp) | ||
| otel.SetTextMapPropagator(propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{})) |
There was a problem hiding this comment.
Does this include the B3 propagation protocol?
There was a problem hiding this comment.
nope, just standard https://www.w3.org/TR/trace-context/ is B3 a hard requirement ?
There was a problem hiding this comment.
Yeah, we want to support both OTEL context propagation and B3. Please install the B3 propagator, too.
|
|
||
| type Config struct { | ||
| LogConfig *LogConfig `yaml:"logging" validate:"required"` | ||
| // TODO: add OTEL config |
There was a problem hiding this comment.
Thoughts out loud.
This auto instrumentation follows a bit different idea than Glide's config.
I would like to see all configuration done via Glide config, so there is one single place to take a look.
Autoinstrumentation creates another place to look at which environment variables which means that now we would have two places to consider in order to fully understand a gateway setup.
Not sure if that's going to be confusing for end users or not.
Functionally though, autoinstrumentation seems to have everything we need:
- ability to disable/enable trace exporting
- ability to disable enable metric exporting (independently from tracing and vice versa)
https://github.com/EinStack/charts/blob/main/charts/glide/templates/_pod.tpl#L32-L50 - the helm chart is ready to accept additional env vars, too.
There was a problem hiding this comment.
Personally I would rather stick with the standards. Users configuring telemetry should only care about their otel setup, we can leverage otel documentation meaning we don't need to upgrade telemetry docs whenever we upgrade our otel packages.
However, one thing I agree with you is the options which are essential for instrumentation. Things like instance id's you mentioned. Those indeed needs to be set in configuration.
There was a problem hiding this comment.
Users configuring telemetry should only care about their otel setup, we can leverage otel documentation meaning we don't need to upgrade telemetry docs whenever we upgrade our otel packages.
I guess it's saving us a bit of typing by providing those registries and implicit configurations via env vars. That's the biggest advantages I see. Also, some configs may appear in the upcoming versions of OTEL SDK that are available right away as we upgrade SDK (without a need to propagate them manually).
|
@roma-glushko I think assigning is for issues not PR. What I want is a review. And I would really appreciate if my PR's descriptions/titles are not modified without notice. Emojis adds cognitive load. |
@gernest did not really get what's the matter with this one?
But PRs have the field, right? 😌 |
@roma-glushko sorry man, my lizard brain had completely forgotten about changelogs, forget what I said.
Yes, but for PR it is used when someone else takes over a PR. If I am the author, it does not make sense to assign me my own work. |
The first major update with breaking changes to the language chat schemas and begging of work on instrumenting the gateway with OpenTelemetry. ### Added - 🔧 Use github.com/EinStack/glide as module name to support go install cmd (@gernest) - ✨🔧 Setup Open Telemetry Metrics and Traces (#237) (@gernest) - 🔧 #221 Add B3 trace propagator (#242) (@gernest) - 🔧 #241 Support overriding OTEL resource attributes (#243) (@gernest) - 🔧 #248 Disable span and metrics by default (#254) (@gernest) - 🔧 #220 Instrument API server with observability signals (#255) (@gernest) - 🔧 #164 Make client connection pool configurable across all providers (#251) (@daesu) - 🔧 Instrument gateway process (#256) (@gernest) - 🔧 #262: adding connection pool for chat request and response (#271) (@tom-fitz) ### Changed - 🔧 #238 Implements human-readable durations in config (#253) (@ppmdo) - 🔧 #266: removing omitempty from response definition (#267) (@tom-fitz) #### Breaking Changes - 🔧 💥 #235: Extended the non-streaming chat error schema with new fields to give clients more context around the error (#236) (@roma-glushko) - 💥 Convert all camelCase config fields to the snake_case in the provider configs (#260) (@roma-glushko) - ✨💥 #153: Allow to pass multiple model-specific param overrides (#264) (@roma-glushko) ### Fixed - 🐛 #217: Set build info correctly in Glide images (#218) (@roma-glushko) ### Security - 🔒 Updated golang to 1.22.4 to address CVE-2024-24790 (#276) (@STAR-173) ### Miscellaneous - 📝 Defined a way to manage EinStack Glide project (#234) (@roma-glushko) - 👷 #219: Setup local telemetry stack with Jaeger, Grafana, VictoriaMetrics and OTEL Collector (#225) (@roma-glushko) - 👷♂️ Added a new GH action to watch for glide activity stream (#239, #244) (@roma-glushko) - ✨ Switched to the new docs (@roma-glushko) - 🔧 #240: Automatically install air (#277, #270) (@ppmdo, @roma-glushko)
Closes #221
We use
go.opentelemetry.io/contrib/exporters/autoexportfor standard loading exporter configurations via env variables.This commit, initialize metrics meter and tracer. There is no need to pass the providers in
*telemetry.Telemetrybecause they are globally available viaotelpackage.Only setup is implemented here to simplify review. Instrumentation will be done in followup patches.