Support distributed trace sampling #52

reachfh · 2022-08-19T23:42:28Z

This PR configures distributed trace sampling according to the new Datadog ingestion controls.

Datadog now uses the x-datadog-sampling-priority header to determine whether a trace should be sampled.
There are four values:

MANUAL_KEEP(2) indicates that the application wants to ensure that a trace is sampled, e.g. if there is an error.
AUTO_KEEP (1) indicates that a trace has been selected for sampling
AUTO_REJECT (0) indicates that the trace has not been selected for sampling
MANUAL_REJECT (-1) indicates that the application wants a trace to be dropped

Before this PR, when initializing a distributed trace via headers, if the x-datadog-sampling-priority header was not set, a default value of 1 was used. This effectively forced all traces to be sampled. Now, if the header is not present, the priority is not set (default is nil).

Distributed traces can set the priority of the trace as follows:

Set the priority based on the x-datadog-sampling-priority header on the inbound request, propagating it to downstream http requests (handled by this module).
If no priority header is present (i.e. this is the first app in the trace), make a sampling decision (e.g. in a Phoenix plug) and set the priority to 1 on the current trace (see Spandex.Tracer.update_priority/2)
Force tracking manually by setting priority to 2 on the trace in application code

The new module SpandexDatadog.RateSampler supports sampling a proportion of traces based on the random trace_id.

In addition, this PR changes the sample rate parameters which are set when sending a trace to Datadog.
Most of these are now obsolete with the new Datadog ingest handling, and only the distributed sampling priority should be used. In the past, values were hard coded to 1.0. Now the default is to not send these metrics, but they can be configured via span tags.

The x-datadog-origin tag is set when traces are started by Datadog RUM and synthetic traces. This PR propagates that header via baggage.

Breaking Changes:

Change default priority to nil (not set) instead of 1
Do not send metric for priority unless set
Do not set x-datadog-sampling-priority header unless priority is set
Use sample_rate tag to set _dd1.sr.eausr value
Use rule_sample_rate tag to set _dd.rule_psr without default (previously hard coded to 1.0)
Use rate_limiter_rate tag to set _dd.limit_psr, without default (previously hard coded to 1.0)
Update library deps
Require a minimum of Elixir 1.11 (was 1.7)

Features:

Use agent_sample_rate tag to set _dd.agent_psr without default (was 1.0)
Add sampler which samples a proportion of traces, setting priority
Handle x-datadog-origin header
Add constants for priority in lib/spandex_datadog/constants.ex
Add GitHub Actions to run tests
Use current Config instead of Mix.Config in config files

GregMefford · 2022-09-07T13:18:07Z

Hey! Thanks a lot for all the effort you put into this PR! ❤️
I am planning to spend some time this week and next to give Spandex some more attention, so I will try to get this reviewed and merged soon.

GregMefford

I started to look through this PR before realizing that I should focus on the spandex PR first. Just committing the comments that I had already collected here.

lib/spandex_datadog/adapter.ex

GregMefford · 2022-09-07T14:35:41Z

mix.exs

@@ -52,9 +52,11 @@ defmodule SpandexDatadog.MixProject do
  defp deps do
    [
      {:msgpax, "~> 2.2.1 or ~> 2.3"},
-      {:spandex, "~> 3.0"},
+      {:spandex, github: "TheRealReal/spandex", branch: "distributed-sampling-priority"},


I guess I should focus on getting https://github.com/spandex-project/spandex/pull/142/files merged first, eh? 😁

Yes, the spandex changes are simpler and enable spandex_datadog. All the dirty work is done here.

GregMefford · 2022-09-07T14:44:36Z

CHANGELOG.md

+* Change default `priority` to `nil` (not set) instead of 1
+* Do not send metric for `priority` unless set
+* Do not set `x-datadog-sampling-priority` header unless priority is set
+* Use `sample_rate` tag to set `_dd1.sr.eausr` value
+* Use `rule_sample_rate` tag to set `_dd.rule_psr` without default (previously hard coded to 1.0)
+* Use `rate_limiter_rate` tag to set `_dd.limit_psr`, without default (previously hard coded to 1.0)


I'm curious if you know whether these breaking changes are necessary or if you're just proposing them. If we could avoid making breaking changes in Spandex, that would obviously be preferred, but if the current behavior is incorrect in terms of what Datadog expects, then we can definitely move toward fixing it.

They are necessary for us to have control so we can reduce our Datadog bill :-).
Forcing the span analytics on results in broken traces for us, i.e. we are sending (and paying for) spans even when the original trace was sampled out and not retained. The span analytics and rule settings are also obsolete with the new Datadog server-based ingestion controls.

Co-authored-by: Greg Mefford <greg@gregmefford.com>

zachdaniel · 2023-02-02T08:06:39Z

Trying this out, but it doesn't seem to work. Not sure why, but we're still seeing 100% of spans being ingested for "unknown" reason which I assume is that the agent didn't think it could sample. @reachfh does this work for you?

kamilkowalski · 2023-04-25T20:57:37Z

@zachdaniel I've verified the changes in this PR to see how it would affect our trace ingestion and created a demo app for testing. Here's how to set it up and documentation on some findings (including a bug): https://github.com/kamilkowalski/spandex_sampling_test#readme

So it seems the sampling works when viewing live traces in Datadog, but it also seems Ingestion Control constantly reports 100% of traces being ingested even when the rate is 0.5.

Maybe we're not sending some necessary statistics or headers? The library is not being recognized by Datadog - could that be a factor?

kamilkowalski · 2023-04-26T08:27:57Z

Okay, Ingestion Control reporting weird numbers is actually documented behavior¹:

Application-side sampling drops traces as early as possible. This causes the Ingestion Controls page to not receive enough information to report accurate sampling rates. Use only when reducing the tracing overhead is paramount.

https://docs.datadoghq.com/tracing/trace_collection/dd_libraries/ruby/#application-side-sampling ↩

kamilkowalski

Thanks for your awesome work @reachfh! I'm not one of the maintainers but I went through the changes and left some comments. We're interested in getting some form of sampling working in Spandex so if there's any way I can help just let me know.

kamilkowalski · 2023-04-26T12:28:54Z

.github/workflows/ci.yml

+      - name: Run tests
+        run: |
+          mix test
+          mix format --check-formatted


mix format doesn't need deps to be installed and could even run in a separate job.

kamilkowalski · 2023-04-26T15:57:39Z

.github/workflows/ci.yml

+          mix local.rebar --force
+          mix local.hex --force


Rebar3 and Hex are installed by erlef/setup-beam by default.

Sounds good

kamilkowalski · 2023-04-26T15:59:34Z

.github/workflows/ci.yml

+        uses: actions/cache@v2
+        with:
+          path: deps/
+          key: deps-${{ runner.os }}-${{ matrix.otp }}-${{ matrix.elixir }}-${{ hashFiles('**/mix.lock') }}


The deps directory only contains dependency source files, no need to depend on anything else than mix.lock:

Suggested change

key: deps-${{ runner.os }}-${{ matrix.otp }}-${{ matrix.elixir }}-${{ hashFiles('**/mix.lock') }}

key: deps-${{ hashFiles('mix.lock') }}

Sounds good

kamilkowalski · 2023-04-26T16:03:58Z

.circleci/config.yml

@@ -1,7 +1,7 @@
-# Elixir CircleCI 2.0 configuration file
-#
+---


I think this could be removed now that there's a GitHub Actions workflow.

I don't use CircleCI, so it's fine with me.

kamilkowalski · 2023-04-26T16:37:26Z

lib/spandex_datadog/constants.ex

+  `priority` value `AUTO_REJECT` (0) indicates that the trace has not been
+  selected for sampling.
+  """
+  def auto_reject, do: 1


Suggested change

def auto_reject, do: 1

def auto_reject, do: 0

kamilkowalski · 2023-04-27T10:59:24Z

lib/spandex_datadog/rate_sampler.ex

@@ -0,0 +1,44 @@
+defmodule SpandexDatadog.RateSampler do


I'm thinking this could implement a more generic Sampler behaviour, and it should be possible to set this sampler as the one used for all traces when doing application-side sampling. This is how dd-trace-rb does it. We could then implement more sampling strategies. Otherwise, every time you start a trace, you will need to run this sampler and set the priority. In our services, traces can be started in HTTP endpoints, gRPC endpoints and Kafka consumers - that's a lot of places to update. Even if it's a middleware, I don't see a reason why there shouldn't be a way to configure a global sampler for all traces starting in the service.

reachfh · 2023-04-27T17:15:13Z

I no longer have access to the TheRealReal repo which was used to create this PR. I can make another PR, or feel free to make whatever changes you like to this one.

reachfh added 24 commits August 8, 2022 20:10

Handle optional sampling priority as nil; handle optional origin header

f5eb665

Baggage keys are atoms

322aa33

Baggage keys are atoms.

fbbebcd

Format test names to start lowercase

b7cb9e8

Set sampling via tags

63bec74

Use Config instead of Mix.Config

7294fa0

Remove parens on functions without parameters

6c98e16

Remove parens on functions without parameters

13f92f4

Add credo and dialyzer.

b21745c

Specify spandex from github

ea15a73

Upgrade libraries

97ebb97

Ignore unrecognized headers

a68debd

Use Keyword module

cbd8977

mix format

8f2075a

mix format

b99404d

Add docs

a73218a

Use float value for analytics rate

508ab18

Document max_id

6fd352b

Add functions to randomly sample traces

2dcbc69

Shortcut for common cases

680acc8

Use float

31d0a9e

Finish rate sampler

27f9d9e

Fix param

132961c

Update changelog

26f3c03

reachfh mentioned this pull request Aug 20, 2022

Support distributed trace sampling spandex-project/spandex#142

Open

reachfh added 5 commits August 22, 2022 11:22

Update docs

2f7c7c3

Fix quotes

f1e64a2

Use new names for priority constants

e0618f2

Add constants for priority

dac254a

Doc

5b3a758

reachfh added 14 commits August 23, 2022 13:27

Add GitHub Actions

bd2f7ac

mix format

2aa894b

Enable --warnings-as-errors

a4c87d1

Add :file and :line to logger metadata for tests

418834d

Run mix format check after deps.get

257451c

Require Elixir 1.11

78c01a3

Test older versions of Elixir

968aa6b

Use setup-elixir instead of setup-beam

ba93460

Use newer version of setup-elixir

4ace750

Specify detailed versions of Elixir and OTP

2c22a7f

Fix variable

7d903be

Use erlef/setup-beam; only build on OTP 24

dd69b30

Remove invalid --warnings-as-errors option

4d018cb

Use latest syntax

6fd650d

GregMefford reviewed Sep 8, 2022

View reviewed changes

Update lib/spandex_datadog/adapter.ex

71cda32

Co-authored-by: Greg Mefford <greg@gregmefford.com>

kamilkowalski suggested changes Apr 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support distributed trace sampling #52

Support distributed trace sampling #52

reachfh commented Aug 19, 2022 •

edited

Loading

GregMefford commented Sep 7, 2022

GregMefford left a comment

GregMefford Sep 7, 2022

reachfh Sep 8, 2022

GregMefford Sep 7, 2022

reachfh Sep 8, 2022

zachdaniel commented Feb 2, 2023

kamilkowalski commented Apr 25, 2023

kamilkowalski commented Apr 26, 2023

kamilkowalski left a comment

kamilkowalski Apr 26, 2023

kamilkowalski Apr 26, 2023

reachfh Apr 27, 2023

kamilkowalski Apr 26, 2023

reachfh Apr 27, 2023

kamilkowalski Apr 26, 2023

reachfh Apr 27, 2023

kamilkowalski Apr 26, 2023

reachfh Apr 27, 2023

kamilkowalski Apr 27, 2023

reachfh commented Apr 27, 2023

	key: deps-${{ runner.os }}-${{ matrix.otp }}-${{ matrix.elixir }}-${{ hashFiles('**/mix.lock') }}
	key: deps-${{ hashFiles('mix.lock') }}

Support distributed trace sampling #52

Are you sure you want to change the base?

Support distributed trace sampling #52

Conversation

reachfh commented Aug 19, 2022 • edited Loading

Breaking Changes:

Features:

GregMefford commented Sep 7, 2022

GregMefford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachdaniel commented Feb 2, 2023

kamilkowalski commented Apr 25, 2023

kamilkowalski commented Apr 26, 2023

Footnotes

kamilkowalski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reachfh commented Apr 27, 2023

reachfh commented Aug 19, 2022 •

edited

Loading