Skip to content

Comments

Traces pre aggregated#2277

Merged
BrynCooke merged 7 commits intodevfrom
bryn/traces_pre_aggregated
Dec 16, 2022
Merged

Traces pre aggregated#2277
BrynCooke merged 7 commits intodevfrom
bryn/traces_pre_aggregated

Conversation

@BrynCooke
Copy link
Contributor

@BrynCooke BrynCooke commented Dec 15, 2022 •
edited
Loading

This new flag on the protobuf means that we no longer have to keep track if we are sending traces or stats. Instead we are asserting that stats are comprehensive and traces are ancillary.

From the router side we no longer skip metrics increment if a trace is being sampled.

@BrynCooke BrynCooke force-pushed the bryn/traces_pre_aggregated branch from 602fc8f to 193bc55 Compare December 15, 2022 16:52
@BrynCooke BrynCooke linked an issue Dec 15, 2022 that may be closed by this pull request
Field references missing on Apollo traces #2267
Closed
@BrynCooke BrynCooke self-assigned this Dec 15, 2022
@BrynCooke BrynCooke requested review from bnjjj and glasser December 15, 2022 16:52
glasser
glasser reviewed Dec 15, 2022
View reviewed changes
glasser
glasser reviewed Dec 15, 2022
View reviewed changes
bnjjj
bnjjj approved these changes Dec 16, 2022
View reviewed changes
Copy link
Contributor

@bnjjj bnjjj left a comment

Choose a reason for hiding this comment

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

LGTM except the typo

@BrynCooke BrynCooke changed the base branch from router_service to dev December 16, 2022 11:52
@BrynCooke BrynCooke force-pushed the bryn/traces_pre_aggregated branch from 1fb4388 to dde2e2a Compare December 16, 2022 12:28
@BrynCooke BrynCooke requested a review from bnjjj December 16, 2022 12:33
@BrynCooke
Copy link
Contributor Author

BrynCooke commented Dec 16, 2022

@bnjjj I made enough additional changes to warrant a re-review. It removes a bunch of Option from config to make accessing values easier, and also makes the Apollo exporter use the batch processor config.

bnjjj
bnjjj approved these changes Dec 16, 2022
View reviewed changes
Copy link
Contributor

@bnjjj bnjjj left a comment

Choose a reason for hiding this comment

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

It's cleaner ! Nice

@BrynCooke BrynCooke merged commit de723f6 into dev Dec 16, 2022
@BrynCooke BrynCooke deleted the bryn/traces_pre_aggregated branch December 16, 2022 15:10
@BrynCooke BrynCooke mentioned this pull request Dec 19, 2022
Merged
BrynCooke pushed a commit that referenced this pull request Dec 19, 2022
Apollo trace requests do not set operation count.
870b3bf 
Metrics is the only way operation count should be incremented.

This change has not been released but the issue was introduced in #2277
Fixes #2267
BrynCooke added a commit that referenced this pull request Dec 19, 2022
@BrynCooke @bnjjj
As we are now using metrics for all requests we should not set operat…
5645b76 
…ion count (#2286)

This change has not been released but the issue was introduced in #2277
Fixes #2267
Metrics will always have the definitive operation count.

Co-authored-by: bryn <bryn@apollographql.com>
Co-authored-by: Coenen Benjamin <benjamin.coenen@hotmail.com>
@abernix abernix added this to the v1.7.0 milestone Dec 22, 2022
@abernix abernix mentioned this pull request Dec 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glasser glasser glasser left review comments

+1 more reviewer

@bnjjj bnjjj bnjjj approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@BrynCooke BrynCooke

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Field references missing on Apollo traces

4 participants

@BrynCooke @glasser @bnjjj @abernix