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

A way to scrub telemetry_ecto events of sensitive data #450

Open
lukad opened this issue Jan 9, 2025 · 6 comments
Open

A way to scrub telemetry_ecto events of sensitive data #450

lukad opened this issue Jan 9, 2025 · 6 comments
Labels
enhancement New feature or request

Comments

@lukad
Copy link

lukad commented Jan 9, 2025

We recently noticed that ecto telemetry events emitted for postgres errors contains the full executed query including the actual values passed to queries or the actual values conflicting with constraints for example.

Here is a screenshot of an example:
ERROR 23505 unique_violation

This is a problem because we don't want to leak sensitive data such as personally identifying information to whatever system consumes these events.

Describe the solution you'd like
I think a builtin way to transform events, which allows library users to implement data scrubbing, would be a good addition to telemetry_ecto.

Our current workaround looks like this, we have our own module that attaches an event handler for repo events. The module has an event handler that forwards all events to OpentelemetryEcto.handle_event but scrubs errors of sensitive data.

defmodule MyApp.OpentelemetryEcto do
  def setup(event_prefix, config \\ []) do
    event = event_prefix ++ [:query]
    :telemetry.attach({__MODULE__, event}, event, &__MODULE__.handle_event/4, config)
  end

  def handle_event(event, measurements, %{result: {:error, error}} = data, config) do
    error = scrub_error(error)
    OpentelemetryEcto.handle_event(event, measurements, %{data | result: {:error, error}}, config)
  end

  def handle_event(event, measurements, data, config) do
    OpentelemetryEcto.handle_event(event, measurements, data, config)
  end

  defp scrub_error(%Postgrex.Error{} = error) do
    ...
  end
end

Ideally OpentelemetryEcto provides a way to specify a module or function that will be called for all handled events to transform them.

The most basic way to achieve this could be OpentelemetryEcto.setup([:my_app, :repo], transform: fn _even -> ... end).

Describe alternatives you've considered
The workaround above works fine, but I believe it's worth having a builtin and documented way of doing this.

What do you think? I'd be happy to contribute a MR.

@lukad lukad added the enhancement New feature or request label Jan 9, 2025
@bryannaegele
Copy link
Collaborator

What happens if you set redact: true on the schema field?

@lukad
Copy link
Author

lukad commented Jan 9, 2025

We already have redact: true on all ecto schema fields we don't want to log. It has no effect here because the errors we're scrubbing are coming from postgrex which doesn't know anything about ecto schemas.

@danschultzer
Copy link
Contributor

You would normally want to scrub it with a processor: https://opentelemetry.io/docs/security/config-best-practices/#scrub-sensitive-data

@lukad
Copy link
Author

lukad commented Jan 15, 2025

I see. I think there is value in scrubbing the data earlier like how I'm doing it because it allows me to scrub the %Postgrex.Error{} structs instead of having to do it in a much more unreliable string based way.
If you think this should not be a direct concern of telemetry_ecto then please feel free to close the issue :)

@tsloughter
Copy link
Member

And since it gets turned into a string you can't even do it with that processor.

@tsloughter
Copy link
Member

Postgrex.Error would have to be flattened to attributes like postgrex.error.<...>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants