feat(outcomes): Emit outcomes as client reports [INGEST-247] #1119

jjbayer · 2021-11-09T20:54:40Z

Add the option to emit outcomes as client reports. This allows external relays to emit outcomes.

Main feature of this PR:

The emit_outcomes config flag can now have three states: true, false, or "client reports".
Every created TrackOutcome is now sent to the outcome aggregator instead of the outcome producer.
If configured to emit outcomes as client reports, the outcome aggregator erases event_id and remote_addr from the outcome.
If an outcome still has an event_id after step 3, it is forwarded to the configured producer without aggregating.
Else, the outcome is aggregated as in Feat(outcomes): Aggregate client reports [INGEST-247] #1118.
Finally, the outcome producer converts the aggregated outcome to a client report and sends it to the upstream as an envelope.

Also in this PR:

Dynamic sampling config is now propagated to untrusted relays, allowing them to apply sampling rules.

…ient-reports-original

jjbayer · 2021-11-15T08:59:21Z

relay-server/src/actors/outcome.rs

+            Outcome::RateLimited(_) => &mut client_report._server_rate_limited_events,
+            _ => {
+                // Cannot convert this outcome to a client report.
+                return Ok(());


Note that only these three outcome types will be emitted. Everything else is silently dropped.

relay-server/src/actors/outcome.rs

jjbayer · 2021-11-15T09:07:26Z

relay-server/src/actors/outcome_aggregator.rs


        // TODO: timestamp validation? (min, max)
-        let offset = timestamp.timestamp() as u64 / self.bucket_interval;
+        let offset = msg.timestamp.timestamp() as u64 / self.bucket_interval;


I assume that we do not need to validate the timestamp here, because

outcomes coming in via client reports have their own validation,

all other outcomes are created by trusted relays.

jjbayer · 2021-11-15T09:08:15Z

relay-server/src/actors/project.rs

@@ -121,6 +121,7 @@ pub struct LimitedProjectConfig {
    pub trusted_relays: Vec<PublicKey>,
    pub pii_config: Option<PiiConfig>,
    pub datascrubbing_settings: DataScrubbingConfig,
+    pub dynamic_sampling: Option<SamplingConfig>,


This change enables dynamic sampling in external relays.

relay-server/src/actors/outcome.rs

relay-config/src/config.rs

relay-general/src/protocol/client_report.rs

relay-server/src/actors/envelopes.rs

relay-server/src/actors/outcome.rs

relay-general/benches/benchmarks.rs

relay-metrics/benches/aggregator.rs

untitaker · 2021-11-16T10:26:09Z

relay-server/src/actors/envelopes.rs

+    type Result = Result<(), ()>;
+}
+
+impl Handler<SendClientReport> for EnvelopeManager {


Since SendEnvelope is pub now, can't this live in the outcome actor?

I tried that (that's why SendEnvelope is public) but bumped into an issue I cannot remember. Is it OK if we keep it here to keep SendClientReport consistent with SendMetrics, and refactor both at a later time?

relay-server/src/actors/outcome.rs

untitaker · 2021-11-16T10:48:01Z

relay-server/src/actors/outcome.rs

+pub struct NonProcessingOutcomeProducer {
+    producer: Option<Recipient<TrackOutcome>>,
+    raw_producer: Option<Recipient<TrackRawOutcome>>,
+}


It should be possible to replace this with

pub enum NonProcessingOutcomeProducer { AsClientReports(Addr<ClientReportOutcomeProducer>), AsOutcomes(Addr<HttpOutcomeProducer>), Disabled, }

and remove impl Handler<TrackOutcome> for HttpProducer>

alternatively we could fold all three actors into one

…ient-reports-original

relay-config/src/config.rs

relay-general/benches/benchmarks.rs

relay-metrics/benches/aggregator.rs

jjbayer · 2021-11-16T16:49:19Z

relay-server/src/actors/envelopes.rs

+    type Result = Result<(), ()>;
+}
+
+impl Handler<SendClientReport> for EnvelopeManager {


I tried that (that's why SendEnvelope is public) but bumped into an issue I cannot remember. Is it OK if we keep it here to keep SendClientReport consistent with SendMetrics, and refactor both at a later time?

jjbayer · 2021-11-17T07:33:29Z

relay-server/src/actors/outcome.rs

+    type Context = Context<Self>;
+
+    fn started(&mut self, ctx: &mut Self::Context) {
+        ctx.run_interval(self.flush_interval, Self::flush);


@untitaker In the end I decided to use run_interval here. The outcomes for ClientReportOutcomeProducer always come from the aggregator in predictable intervals, so I suppose there is no need for "flush when batch is full or too much time has passed" logic. Instead, just flush with the same interval as the aggregator.

jjbayer · 2021-11-17T07:35:31Z

relay-config/src/config.rs

@@ -1008,7 +1090,7 @@ pub struct Outcomes {
 impl Default for Outcomes {
    fn default() -> Self {
        Outcomes {
-            emit_outcomes: false,
+            emit_outcomes: EmitOutcomes::None,


Should we set the default to EmitOutcomes::AsClientReports?

i think so, in that case we should write some warnings into the changelog imo

relay-config/src/config.rs

untitaker · 2021-11-17T12:07:52Z

relay-config/src/config.rs

@@ -1008,7 +1090,7 @@ pub struct Outcomes {
 impl Default for Outcomes {
    fn default() -> Self {
        Outcomes {
-            emit_outcomes: false,
+            emit_outcomes: EmitOutcomes::None,


i think so, in that case we should write some warnings into the changelog imo

untitaker · 2021-11-17T12:09:44Z

relay-config/src/config.rs

+        if self.processing_enabled() {
+            return EmitOutcomes::AsOutcomes;
+        }
+        self.values.outcomes.emit_outcomes


@jjbayer this makes me think, maybe we should make the field in the config Option<EmitOutcomes> so explicitly setting the value to anything takes precedence over this. but not entirely sure. can leave it out for now

untitaker · 2021-11-17T12:10:32Z

relay-filter/src/common.rs

@@ -162,6 +162,26 @@ impl fmt::Display for FilterStatKey {
    }
 }

+impl<'a> TryFrom<&'a str> for FilterStatKey {


Don't we have a macro for this? maybe just for serde impl

relay-metrics/benches/aggregator.rs

relay-server/src/actors/outcome.rs

untitaker

I think we should flip the default before we merge this, because people are not going to enable this explicitly. All other comments are optional.

…ient-reports-original

* master: test(outcomes): Fix sort order in flaky test (#1135) feat(outcomes): Aggregate more outcomes (#1134) ref(outcomes): Fold processing vs non-processing into single actor (#1133) build: Update symbolic to support UE5 (#1132) feat(metrics): Extract measurement ratings, port from frontend (#1130) feat(metrics): Another statsd metric to measure bucket duplication (#1128) feat(outcomes): Emit outcomes as client reports (#1119) fix: Move changelog line to right version (#1129) fix(dangerjs): Do not suggest to add JIRA ticket to changelog (#1125) feat(metrics): Tag metrics by transaction name [INGEST-542] (#1126)

jjbayer added 24 commits November 5, 2021 14:54

wip

57ff26e

wip

4fad457

wip

ed93929

fix: processing flow

64d6d5f

wip

2bdab2c

feat(outcomes): Aggregate client reports in outcome aggregator

a519519

test: Test aggregation in integration test

e15501f

ref: Cleanup + doc

9496481

Add statsd metric for outcome aggregator flush time

c2dd6ea

ref: Remove old comment

91a631c

fix: Increase mailbox size

64749c0

Add changelog

e0214ed

ref: Use or_default instead of or_insert_with

29581bc

Merge branch 'feat/aggregate-client-reports' into feat/outcomes-as-cl…

d9a2ad9

…ient-reports-original

feat: Make aggregation a noop if event_id is set

2d73cee

feat: Send client report envelopes from outcomes

d5556a9

wip

20e01de

feat: Add outcome to client reports

f6179bc

feat: Disable outcome aggregator if not needed

e9741f9

feat: Erase event_id and remote_addr for lossy outcome aggregation

01c034a

Remove Accepted variant of Outcome enum

73681af

wip

e97d4b6

test: Use dynamic sampling in integration test

19c9333

Merge branch 'master' into feat/outcomes-as-client-reports-original

3f9c410

jjbayer changed the title ~~feat(outcomes): Emit outcomes as client reports~~ feat(outcomes): Emit outcomes as client reports [INGEST-247] Nov 12, 2021

jjbayer added 5 commits November 12, 2021 08:46

ref(config): tri-state emit outcomes config

bc2c141

ref: cleanup

85a6587

Add statsd tag name for client reports

5646935

ref: Add new fields to client report (one per outcome type)

c73e9f5

ref: Custom serializer for EmitOutcomes

0c6c629

jjbayer added 3 commits November 12, 2021 16:41

feat: Allow dynamic sampling config to propagate to external relays

1576c29

test: Fix integration test

d534c1c

fix: tests and clippy

26767f2

jjbayer marked this pull request as ready for review November 15, 2021 09:17

jjbayer requested review from a team and untitaker November 15, 2021 09:17

jjbayer commented Nov 15, 2021

View reviewed changes

untitaker reviewed Nov 16, 2021

View reviewed changes

jjbayer added 6 commits November 16, 2021 19:30

ref: Apply review feedback

8bf84d9

feat: Batch client reports before sending as envelopes

d602615

feat: Complete Outcome::from_outcome_type

a09c68a

fix: Use same interval for client report batch as for outcome aggregator

c8e1792

Merge remote-tracking branch 'origin/master' into feat/outcomes-as-cl…

57401b4

…ient-reports-original

Add changelog

84be868

jjbayer commented Nov 17, 2021

View reviewed changes

jjbayer requested a review from untitaker November 17, 2021 07:36

untitaker reviewed Nov 17, 2021

View reviewed changes

untitaker requested changes Nov 17, 2021

View reviewed changes

Apply review feedback

d2a9f19

jjbayer requested a review from untitaker November 17, 2021 18:41

Merge remote-tracking branch 'origin/master' into feat/outcomes-as-cl…

b13d136

…ient-reports-original

untitaker approved these changes Nov 18, 2021

View reviewed changes

jjbayer merged commit d242158 into master Nov 18, 2021

jjbayer deleted the feat/outcomes-as-client-reports-original branch November 18, 2021 10:21

jan-auer mentioned this pull request Feb 19, 2022

feat(relay): Document outcomes endpoint for request routing [INGEST-973] getsentry/sentry-docs#4748

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(outcomes): Emit outcomes as client reports [INGEST-247] #1119

feat(outcomes): Emit outcomes as client reports [INGEST-247] #1119

jjbayer commented Nov 9, 2021 •

edited

Loading

jjbayer Nov 15, 2021

jjbayer Nov 15, 2021

jjbayer Nov 15, 2021

untitaker Nov 16, 2021

jjbayer Nov 16, 2021

untitaker Nov 16, 2021

jjbayer Nov 16, 2021

jjbayer Nov 17, 2021

jjbayer Nov 17, 2021

untitaker Nov 17, 2021

untitaker Nov 17, 2021

untitaker Nov 17, 2021

untitaker Nov 17, 2021

untitaker left a comment •

edited

Loading

feat(outcomes): Emit outcomes as client reports [INGEST-247] #1119

feat(outcomes): Emit outcomes as client reports [INGEST-247] #1119

Conversation

jjbayer commented Nov 9, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker left a comment • edited Loading

Choose a reason for hiding this comment

jjbayer commented Nov 9, 2021 •

edited

Loading

untitaker left a comment •

edited

Loading