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

Jump to bottom

add support of metrics #738

Merged
merged 21 commits into from Apr 6, 2022
Merged

add support of metrics #738

merged 21 commits into from Apr 6, 2022

Conversation

bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Mar 25, 2022 •
edited

I spotted some bugs I fixed:

  • I now trim the query to check if the query is empty or not, before we could trick the layer by using query=" "
  • I put the original context in the RouterResponse when it's returning an error
  • close tower buffers of 20_000 should be simplified #772
  • Fix documentation issues

@bnjjj
add support of metrics WIP
6d00700 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@github-actions

This comment has been minimized.

Sign in to view
@netlify
Copy link

netlify bot commented Mar 25, 2022 •
edited

✅ Deploy Preview for apollo-router-docs canceled.

Name Link
🔨 Latest commit 807e809
🔍 Latest deploy log https://app.netlify.com/sites/apollo-router-docs/deploys/624c683141d9a40009f9e918

@bnjjj
add support of metrics WIP
b2f35fe 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
add custom_endpoint method in plugins
4dbfd76 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
dig into oltp implementation
bdfa49d 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
extend RouterServiceFactory trait
eeb9ca7 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@BrynCooke
Copy link
Contributor

@garypen had the idea that a plugin may expose a service at a predictable path. This seems like a better idea than fully dynamic routing. Then if plugins want to expose multiple endpoints they can use steer to route to different paths internally..

@bnjjj
use service instead of closure for handlers
6f22710 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
garypen
garypen reviewed Mar 31, 2022
View reviewed changes
Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

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

Looks good. I've got a few NIT style comments, but the main bit I don't understand is the question about the otel metrics_exporter. Perhaps we can talk about that in huddle?

apollo-router-core/src/plugin.rs Outdated Show resolved Hide resolved
apollo-router-core/src/services/mod.rs Show resolved Hide resolved
apollo-router/src/plugins/telemetry/metrics.rs Outdated Show resolved Hide resolved
apollo-router/src/plugins/telemetry/otlp/mod.rs Outdated Show resolved Hide resolved
apollo-router/src/state_machine.rs Outdated Show resolved Hide resolved
apollo-router/src/state_machine.rs Outdated Show resolved Hide resolved
apollo-router/src/warp_http_server_factory.rs Outdated Show resolved Hide resolved
@bnjjj
fixes
57888cc 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
fix tests
9fc78ec 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
Merge branch 'main' of github.com:apollographql/router into bnjjj/fea…
6beaa45 
…t_84
@bnjjj
add more metrics
8fd39c7 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
refactor: simplify the usage of buffer with 20_000 param #772
7ddab16 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj linked an issue Apr 1, 2022 that may be closed by this pull request
Event metrics #84
Closed
@bnjjj
add otlp in progress
5f2fb7f 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
add more metrics
e08ea86 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
add github issue comment
73f66c7 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
Merge branch 'main' of github.com:apollographql/router into bnjjj/fea…
f7cede8 
…t_84
@bnjjj
fix ci
7463ac7 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj marked this pull request as ready for review April 4, 2022 16:00
@bnjjj bnjjj self-assigned this Apr 4, 2022
@bnjjj
docs: fix plugin name and add new documentation for metrics
288eab0 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj changed the title add support of metrics WIP add support of metrics Apr 5, 2022
o0Ignition0o
o0Ignition0o reviewed Apr 5, 2022
View reviewed changes
Copy link
Contributor

@o0Ignition0o o0Ignition0o left a comment

Choose a reason for hiding this comment

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

I probably missed a conversation so I m going to ask here: did we revert from telemetry: to plugins / apollo.telemetry: ? why ? ^^'

docs/source/configuration/header-propagation.mdx Outdated Show resolved Hide resolved
docs/source/configuration/header-propagation.mdx Outdated Show resolved Hide resolved
@bnjjj
docs: revert bad fix
dfb4e48 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
o0Ignition0o
o0Ignition0o approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@o0Ignition0o o0Ignition0o left a comment

Choose a reason for hiding this comment

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

A few comments here and there, but those are more questions, it LGTM overall.

Solid work btw, that's uh quite a big pull request, so GG 🎉

apollo-router-core/src/plugins/traffic_shaping.rs Show resolved Hide resolved
apollo-router/src/plugins/telemetry/metrics.rs Show resolved Hide resolved
apollo-router/src/plugins/telemetry/metrics.rs Show resolved Hide resolved
apollo-router/src/plugins/telemetry/mod.rs Outdated Show resolved Hide resolved
apollo-router/src/plugins/telemetry/otlp/mod.rs Show resolved Hide resolved
@bnjjj
pub crate for module in telemetry
a414814 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
address comments from review
807e809 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj requested a review from Geal April 5, 2022 16:03
@bnjjj bnjjj merged commit 3bc0007 into main Apr 6, 2022
@bnjjj bnjjj deleted the bnjjj/feat_84 branch April 6, 2022 08:32
@Geal Geal mentioned this pull request Apr 6, 2022
Closed
@Geal Geal added this to the v0.1.0-preview.3 milestone Apr 8, 2022
@bnjjj bnjjj mentioned this pull request Aug 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrynCooke BrynCooke BrynCooke left review comments

@o0Ignition0o o0Ignition0o o0Ignition0o approved these changes

@Geal Geal Geal approved these changes

@garypen garypen Awaiting requested review from garypen

Assignees

@bnjjj bnjjj

Labels
component/plugins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tower buffers of 20_000 should be simplified Event metrics
5 participants
@bnjjj @BrynCooke @garypen @Geal @o0Ignition0o