Reporting followups #1020

BrynCooke · 2022-05-11T15:27:04Z

This is WIP. I still need to add executable_schema_id, this will be the most difficult part.

Spaceport now just deals with reports and forwards them on. There is no aggregation taking place in spaceport.
The telemetry plugin now composes the complete report for studio.
Metrics are aggregated on Key which is now an enum of excluded or regular. Excluded has no information but allows us to get operation_count.
Report header is introduced so that we can eventually populate it with all the right info.

@glasser Be good if you could take a look to see if this going in the right direction.

Re #898 (comment)

…the map. #898 (comment)

#898 (comment)

netlify · 2022-05-11T15:27:11Z

✅ Deploy Preview for apollo-router-docs canceled.

Name	Link
🔨 Latest commit	`7f3ffaf`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-router-docs/deploys/627d2f0e4aa6230009ac8ec5

github-actions · 2022-05-11T15:27:29Z

@BrynCooke your pull request is missing a changelog!

Spaceport now just deals with reports and forwards them on. There is no aggregation taking place in spaceport. The telemetry plugin now composes the complete report for studio. Metrics are aggregated on Key which is now an enum of excluded or regular. Excluded has no information. Report header is introduced so that we can eventually populate it with all the right info.

glasser

looking nice...

apollo-router/src/plugins/telemetry/metrics/apollo.rs

glasser · 2022-05-11T23:13:00Z

apollo-router/src/plugins/telemetry/mod.rs

+                }
+            }
+        } else {
+            // Usage reporting was missing, so it counts as one operation.


not sure i understand where this ends up but 🤷

A malicious plugin could remove the usage reporting from context in order to try avoid op count increasing.

apollo-router-core/src/spec/schema.rs

garypen

Looks cleaner than the old code. Nice work!

garypen · 2022-05-12T12:37:26Z

apollo-router-core/src/spec/schema.rs

+        #[cfg(windows)]
+        assert_eq!(
+            schema.schema_id,
+            Some("C081A7FF570F630BDF22CB6CA73A2BB1D46657EF69FC66749F5A1F99332B5ECB".to_string())
+        );
+        #[cfg(not(windows))]
+        assert_eq!(
+            schema.schema_id,
+            Some("6881633761291F4A6E4F9A4A6BBE7AD69A80C63EBEBFC357464B7A6EF276EF0A".to_string())


re: sha256 and windows vs not windows. I guess this is only likely to be an issue for local schemas. I mean if you get it from uplink (majority of prod use cases I guess) then the source text should match exactly…

@zionts executable_schema_id sha is different on windows currently because of line endings. Would you prefer that this is normalized?

Where does the line ending change get introduced? Is it as part of checking out the file with git, or reading it in with a Rust API? Seems like there must be a way to just not do that? Like, this isn't actually something that happens if you use Router with Uplink, just related to "there's a graphql file you're raeding from disk", right?

The normal Gateway/Uplink use case (and presumably the normal Router use case?) doesn't involve ever reading/writing the file from disk or doing anything that would do newline conversion...

Interesting. Currently it is possible to use a local schema file in the router and also upload stats. Is this something we should disable?

i'm sure it's fine, i'm just saying that i wouldn't recommend any normalization that takes the string returned from uplink and normalizes it before hashing it.

apollo-router/src/plugins/telemetry/apollo.rs

apollo-router/src/plugins/telemetry/metrics/apollo.rs

apollo-router/src/plugins/telemetry/mod.rs

apollo-router/src/router_factory.rs

apollo-spaceport/src/server.rs

glasser

This looks great (except for the SHA case thing).

The one thing I'm still somewhat confused about is why you have the two separate sets of Report and AggregatedReport types. Is there a reason you can't just use an AggregatedReport for everything? It seems like the only thing you can do with a Report that you can't do with an AggregatedReport is read back the operation's latency as a precise Duration (whereas it's bucketed when you put it in AggregatedReport). But... is that an operation you ever do? Seems like you could cut the types in half.

(And if you do want to keep both types, it would seem a bit easier to understand if the types you currently called AggregatedX were called X and the types you currently call X were called SingleOperationX or something — since that would align the types more directly to the protobuf names.)

That said there's nothing buggy with having the two separate sets of types — it just strikes me as unnecessary.

glasser · 2022-05-12T13:19:29Z

apollo-router/src/plugins/telemetry/metrics/apollo/studio.rs

+
+#[derive(Default, Debug, Serialize)]
+pub(crate) struct AggregatedContextualizedStats {
+    context: StatsContext,


Doesn't this struct live in a HashMap<StatsContext, AggregatedContextualizedStats> already? Seems like this field is redundant? Note that in protobufs, map keys can only be strings or integers, which is we we have a repeated ContextualizedStats with a context field on it instead of a map. But you're already using a map here (since rust apparently supports more general map keys) so this seems like it should be redundant?

I guess you'd need to change the definition of

impl From<AggregatedTracesAndStats> for apollo_spaceport::TracesAndStats {

to create stats_with_context from map entries rather than values.

To use only one set of types we would need to put the aggregated report in a mutex and swap it out on each send. This is an option that we could have used, but maybe it is safer not to do that in case of contention. We can revisit another time.

Let's do any renaming at the end once we are sure everything is working.

Doesn't this struct live in a HashMap<StatsContext, AggregatedContextualizedStats> already? Seems like this field is redundant? Note that in protobufs, map keys can only be strings or integers, which is we we have a repeated ContextualizedStats with a context field on it instead of a map. But you're already using a map here (since rust apparently supports more general map keys) so this seems like it should be redundant?

I guess you'd need to change the definition of

impl From<AggregatedTracesAndStats> for apollo_spaceport::TracesAndStats {

to create stats_with_context from map entries rather than values.

This won't work if I want to use:

impl From<AggregatedContextualizedStats> for apollo_spaceport::ContextualizedStats

as From does not have access to the map entry that it was stored in.

For consistency with the rest of the conversion logic lets keep it as is for now and look again if it is a perf problem.

Renames in 9d71c25

Happy to trust you on this one! Maybe add a comment noting the redundancy if you think that's helpful.

Re

To use only one set of types we would need to put the aggregated report in a mutex and swap it out on each send. This is an option that we could have used, but maybe it is safer not to do that in case of contention. We can revisit another time.

I don't think I understand this one — it seems like the AggregatedReport type can be used anywhere the Report type can be used, except if you want to be able to put a duration into it and pull it out again without rounding. But my feedback is merely about clarity/simplicity/code size, and I very well may be missing something important. No need to convince me of this, happy to trust you're making the right call for today and quite possibly for tomorrow as well.

What I'd really like is that the generated rs files for the protobuf somehow can use the Histogram type. Didn't get time to investigate this though.

I think in the long run we'll make the changes to the datastructure to convert many of the u64s to bools as you suggested earlier, so maybe things will diverge in the long run.

Yeah, in JS I ended up trying to add a feature to to protobuf library that let you use other representations in-memory for repeated fields that would get converted to arrays at encoding time (and then forking the library when it was clearly unmaintained)

garypen

We need to assess the impact of removing report aggregation from spaceport. Hard to determine that from code review.

bryn added 3 commits May 11, 2022 10:22

Add panic if trace reporting is detected

be06557

Re #898 (comment)

Remove merge of referenced_fields_by_type, instead take ownership of …

f87f6d0

…the map. #898 (comment)

Set requests_without_field_instrumentation equal to request_count

e028faa

#898 (comment)

BrynCooke force-pushed the bryn/reporting-followups branch from cfa89a5 to d5b8aec Compare May 11, 2022 15:29

BrynCooke requested a review from glasser May 11, 2022 15:30

BrynCooke force-pushed the bryn/reporting-followups branch from d5b8aec to 48c4d9c Compare May 11, 2022 15:34

BrynCooke changed the title ~~Bryn/reporting followups~~ Reporting followups May 11, 2022

BrynCooke linked an issue May 11, 2022 that may be closed by this pull request

Studio reporting followups #1012

Closed

glasser reviewed May 11, 2022

View reviewed changes

bryn added 9 commits May 12, 2022 09:08

Update test snapshots

c6e1dd2

Add schema id

abb3566

Clippy

9704c9d

Get rid of unnecessary timestamp handling

4e8e3b4

Fix schema hash for windows

6a4b407

Move duration histogram into a submodule of apollo metrics.

3a20d8b

Split apollo.rs into smaller modules.

f8a6cd8

Use default for some fields

4510527

Populate most of the headers information.

9aedc1d

BrynCooke force-pushed the bryn/reporting-followups branch from 5361cfd to 9aedc1d Compare May 12, 2022 10:19

bryn added 2 commits May 12, 2022 14:05

Align router structures with proto

6708381

Clippy

86adf67

glasser reviewed May 12, 2022

View reviewed changes

apollo-router-core/src/spec/schema.rs Outdated Show resolved Hide resolved

garypen requested changes May 12, 2022

View reviewed changes

glasser approved these changes May 12, 2022

View reviewed changes

bryn added 3 commits May 12, 2022 16:54

Fix and test schema_id_injection

2757d4c

Lowercase schema_id

c70be5b

Take in comments

3dd9b24

bryn added 3 commits May 12, 2022 17:35

Renames

9d71c25

Fix tests

19c0c88

Made schema_id mandatory.

702880e

BrynCooke requested a review from garypen May 12, 2022 15:56

Improve lowercase hex formatting

7f3ffaf

garypen approved these changes May 13, 2022

View reviewed changes

BrynCooke merged commit 53e49e5 into main May 13, 2022

BrynCooke deleted the bryn/reporting-followups branch May 13, 2022 06:45

BrynCooke self-assigned this May 13, 2022

Geal added this to the v0.9.0 milestone May 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reporting followups #1020

Reporting followups #1020

BrynCooke commented May 11, 2022 •

edited

netlify bot commented May 11, 2022 •

edited

github-actions bot commented May 11, 2022

glasser left a comment

glasser May 11, 2022

BrynCooke May 12, 2022 •

edited

garypen left a comment

garypen May 12, 2022

BrynCooke May 12, 2022

glasser May 12, 2022

BrynCooke May 12, 2022

glasser May 12, 2022

glasser left a comment

glasser May 12, 2022

BrynCooke May 12, 2022 •

edited

BrynCooke May 12, 2022 •

edited

BrynCooke May 12, 2022

glasser May 12, 2022

glasser May 12, 2022

BrynCooke May 12, 2022

glasser May 12, 2022

garypen left a comment

Reporting followups #1020

Reporting followups #1020

Conversation

BrynCooke commented May 11, 2022 • edited

netlify bot commented May 11, 2022 • edited

✅ Deploy Preview for apollo-router-docs canceled.

github-actions bot commented May 11, 2022

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrynCooke May 12, 2022 • edited

Choose a reason for hiding this comment

garypen 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

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrynCooke May 12, 2022 • edited

Choose a reason for hiding this comment

BrynCooke May 12, 2022 • edited

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

garypen left a comment

Choose a reason for hiding this comment

BrynCooke commented May 11, 2022 •

edited

netlify bot commented May 11, 2022 •

edited

BrynCooke May 12, 2022 •

edited

BrynCooke May 12, 2022 •

edited

BrynCooke May 12, 2022 •

edited