Skip to content
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

chore (sync service): inject OTEL attributes for inclusion in traces within core Electric stacks #2077

Closed
wants to merge 10 commits into from

Conversation

kevin-dp
Copy link
Contributor

@kevin-dp kevin-dp commented Dec 2, 2024

This PR modifies core Electric such that stacks and the serve shape plug can take OTEL attributes to include in every span. As such, we can for instance inject the database_id attribute without requiring core Electric to be tenant aware. I don't include a changeset as this isn't user-facing.

Copy link

netlify bot commented Dec 2, 2024

Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit 9e05bc5
🔍 Latest deploy log https://app.netlify.com/sites/electric-next/deploys/674d9f28ae812f0008f0d30a
😎 Deploy Preview https://deploy-preview-2077--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kevin-dp kevin-dp force-pushed the kevin/inject-otel-attrs-in-stack branch from 97126b6 to 9e05bc5 Compare December 2, 2024 11:51
Copy link
Contributor

@robacourt robacourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for this approach. I don't like how many changes it makes to normal code in order to do the tracing. Personally I think we should go for a more aspect oriented approach. But perhaps that can be a later PR.

@KyleAMathews
Copy link
Contributor

Thanks Kevin!

On alternative approaches — "baggage" is an otel idea for passing around key/value pairs that can added easily as attributes. If the lib we're using has support for that then it's a good option as well https://opentelemetry.io/docs/concepts/signals/baggage/

@kevin-dp
Copy link
Contributor Author

kevin-dp commented Dec 2, 2024

Thanks Kevin!

On alternative approaches — "baggage" is an otel idea for passing around key/value pairs that can added easily as attributes. If the lib we're using has support for that then it's a good option as well https://opentelemetry.io/docs/concepts/signals/baggage/

Seems to be supported: https://hexdocs.pm/opentelemetry_api/0.5.0/OpenTelemetry.Baggage.html

@kevin-dp
Copy link
Contributor Author

kevin-dp commented Dec 2, 2024

So there are a couple of possibilities:

  • keep it like it is by explicitly passing the attributes around
  • "Pass" the attributes around using Baggage (https://opentelemetry.io/docs/concepts/signals/baggage/)
  • Set the attributes using Process.put and then read them in with_span to add them to the span
  • Use a persisted term per stack that stores the OTEL attributes. Read it in with_span and add them to the span.

@alco
Copy link
Member

alco commented Dec 2, 2024

Baggage is a key-value mapping that exists alongside attributes, right @KyleAMathews?

If we want to define a set of attributes that is used by every span, we can set resource attributes in the configuration. We're already doing that for service.name, service.version and instance.id attributes, configured with ELECTRIC_SERVICE_NAME and ELECTRIC_INSTANCE_ID options.

OTEL_RESOURCE_ATTRIBUTES is a standard configuration option in opentelemetry that can be set using env var.

To support per-tenant attributes, I would go with the approach where they are stored as a persistent term during the startup of the tenant supervision tree and are then read inside OpenTelemetry.with_span. We need to create a separate tracer for each tenant and pass it explicitly to OpenTelemetry calls. Right now, the opentelemetry library is used as if there's only one tracing environment for the whole Elixir app.

@KyleAMathews
Copy link
Contributor

Baggage is a generic way to pass around key/value pairs but you still have to explicitly add it to each attribute.

So actually if we want a way to say "these key/values are added to every span automatically" then we'd want to do something ourselves anyways.

@kevin-dp
Copy link
Contributor Author

kevin-dp commented Dec 3, 2024

Closing in favor of #2083 which seems to be a better approach.

@kevin-dp kevin-dp closed this Dec 3, 2024
kevin-dp added a commit that referenced this pull request Dec 4, 2024
… term (#2083)

This PR is an alternative to
#2077.
Instead of injecting the OTEL attributes and passing them around
everywhere, we store each stack's OTEL attributes in a dedicated
persistent term. When creating a span we lookup the OTEL attributes for
that stack and add them to the span.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants