Simplify implementation of telementry's events by IvanGoncharov · Pull Request #7280 · apollographql/router

IvanGoncharov · 2025-04-16T07:08:04Z

I made a few attempts to make minimal fixes based on #7231. However, it was extremely hard to cover all edge cases, so I decided to do a separate refactoring PR first.

The core issue is that currently, some on_request/on_response functions exit earlier:

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

Lines 261 to 264 in 3ae5f3f

    
           if self.request.level() != EventLevel::Off { 
        
               if let Some(condition) = self.request.condition() { 
        
                   if condition.lock().evaluate_request(request) != Some(true) { 
        
                       return;

And because of it, bypass response and custom events:

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

Lines 273 to 281 in 3ae5f3f

    
           if self.response.level() != EventLevel::Off { 
        
               request 
        
                   .context 
        
                   .extensions() 
        
                   .with_lock(|ext| ext.insert(DisplayRouterResponse(true))); 
        
           } 
        
           for custom_event in &self.custom { 
        
               custom_event.on_request(request); 
        
           }

Simply removing returns and implementing logic using ifs also doesn't work:
https://github.com/apollographql/router/pull/7231/files#diff-fab80b4af0ee70850af9ac42bf7990d09912b65f75b39456c6e47ca817d4a0d2R262-R269

It results in the standard event without the condition being ignored.
Trying to address it with a more complicated condition would work, but it would require adding it to more than a dozen places and also doesn't guarantee that we don't break some other edge cases.

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Note any exceptions here

Notes

It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

svc-apollo-docs · 2025-04-16T07:08:08Z

✅ Docs preview ready

The preview is ready to be viewed. View the preview

File Changes

0 new, 6 changed, 0 removed

* graphos/routing/(latest)/observability/telemetry/instrumentation/events.mdx
* graphos/routing/(latest)/observability/telemetry/log-exporters/overview.mdx
* graphos/routing/(latest)/observability/telemetry/metrics-exporters/overview.mdx
* graphos/routing/(latest)/observability/telemetry/trace-exporters/overview.mdx
* graphos/routing/(latest)/security/jwt.mdx
* graphos/routing/(latest)/security/authorization.mdx

Build ID: a75f02e6fe7a2205f586a04d

URL: https://www.apollographql.com/docs/deploy-preview/a75f02e6fe7a2205f586a04d

github-actions · 2025-04-16T07:08:17Z

@IvanGoncharov, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

IvanGoncharov · 2025-04-16T12:53:04Z

...uter/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap

      "type": "object"
    },
-    "EventLevel": {
+    "EventLevelConfig": {


Whole change is caused by my crating separate enum type just for config

apollo-router/src/plugins/telemetry/config_new/connector/events.rs

IvanGoncharov · 2025-04-16T13:10:38Z

apollo-router/src/plugins/telemetry/config_new/connector/events.rs

-                }),
-            }),
-        })
+        .filter_map(|(name, config)| CustomEvent::from_config(name, config))


These functions are very repetitive, so I just moved the code to from_config methods. We can't use From or TryFrom trait here since this method returns options.
I would appreciate suggestions on how to name it better.

Maybe we could still use TryFrom and convert it into an option using .ok()

TryFrom returns Result: https://doc.rust-lang.org/std/convert/trait.TryFrom.html
So it will require us to define some dummy error type.

basically we return None if event is Off
So it's not an error

IvanGoncharov · 2025-04-16T13:12:20Z

apollo-router/src/plugins/telemetry/config_new/connector/events.rs

-    type EventResponse = ();
-
-    fn on_request(&self, request: &Self::Request) {
+impl CustomEvents<ConnectorRequest, ConnectorResponse, (), ConnectorAttributes, ConnectorSelector> {


I removed the Instrumented trait from here, and it worked without any other code modifications.
Doing this allowed me to change on_request so it now gets a mutable reference.

IvanGoncharov · 2025-04-16T13:14:11Z

apollo-router/src/plugins/telemetry/config_new/connector/events.rs

-                .context
-                .extensions()
-                .with_lock(|lock| lock.insert(ConnectorEventRequest(self.request.clone())));
+        if let Some(request_event) = self.request.take() {


I use Option::take so event is basically moved into context and we don't need to call clone here.

IvanGoncharov · 2025-04-16T13:15:53Z

apollo-router/src/plugins/telemetry/config_new/connector/events.rs

+            request.context.extensions().with_lock(|lock| {
+                lock.insert(ConnectorEventRequest {
+                    level: request_event.level,
+                    condition: Arc::new(Mutex::new(request_event.condition)),


As noted earlier I don't think we really need this wrapping, but I don't have time to check so I move it here just to be 100% sure I'm not introducing regression here.

IvanGoncharov · 2025-04-16T13:17:14Z

apollo-router/src/plugins/telemetry/config_new/connector/events.rs

        }

-        for custom_event in &self.custom {
+        for custom_event in &mut self.custom {


Everything is mutable now because I removed the internal wrapping of Arc + Mutex around the condition.

IvanGoncharov · 2025-04-16T13:17:44Z

apollo-router/src/plugins/telemetry/config_new/connector/events.rs


-    fn on_response(&self, response: &Self::Response) {
-        for custom_event in &self.custom {
+    pub(crate) fn on_response(&mut self, response: &ConnectorResponse) {


Need to make it public since I removed trait

IvanGoncharov · 2025-04-16T13:21:16Z

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

-    pub(super) error: StandardEvent<Sel>,
+    pub(super) request: Option<StandardEvent<Sel>>,
+    pub(super) response: Option<StandardEvent<Sel>>,
+    pub(super) error: Option<StandardEvent<Sel>>,


I normalize data here, previously we always had these values but internally they could have EventLevel::Off and this check was noisy.
So I removed Off from EventLevel and just set None instead.

IvanGoncharov · 2025-04-16T13:40:53Z

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

+    pub(super) request: Option<StandardEvent<Sel>>,
+    pub(super) response: Option<StandardEvent<Sel>>,
+    pub(super) error: Option<StandardEvent<Sel>>,
    pub(super) custom: Vec<CustomEvent<Request, Response, EventResponse, Attributes, Sel>>,


Also it removes useless code with custom events, because custom is filtered to not have events that are Off but because EventLevel contained this value it resulted in useless checks

IvanGoncharov · 2025-04-16T13:41:40Z

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

-                if condition.lock().evaluate_request(request) != Some(true) {
-                    return;
-                }
+impl CustomEvents<router::Request, router::Response, (), RouterAttributes, RouterSelector> {


Also removed Instrumented here and it just required me to add pub(crate) to methods

IvanGoncharov · 2025-04-16T13:43:16Z

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

+    pub(crate) fn on_response(&mut self, response: &router::Response) {
+        if let Some(response_event) = &mut self.response {
+            if !response_event.condition.evaluate_response(response) {
+                return;


note: this PR doesn't fix early exit issues I talked about in PR description.
I will fix them in separate PR

IvanGoncharov · 2025-04-16T13:48:29Z

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

-            Self::Conditional { level, .. } => *level,
-        }
-    }
+#[derive(Debug)]


Note: It doesn't contain Clone, previously it was done by wrapping condition into Arc + Mutex, but I changed the code to maintain a single copy inside telemetry plugin.
I still wrap the condition in Arc + Mutex when the condition is passed outside of the telemetry plugin.

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

IvanGoncharov · 2025-04-16T14:05:57Z

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

+    Info,
+    Warn,
+    Error,
+}


Main purpose of adding it is to remove Off

IvanGoncharov · 2025-04-16T14:07:20Z

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

    pub(super) event_on: EventOn,
    pub(super) message: Arc<String>,
-    pub(super) selectors: Option<Arc<Extendable<A, T>>>,
+    pub(super) selectors: Arc<Extendable<A, T>>,


I don't understand why it was wrapped in Option, but it caused issues with my changes so I just removed it.

IvanGoncharov · 2025-04-16T14:09:34Z

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

-    pub(super) inner: Mutex<CustomEventInner<Request, Response, EventResponse, A, T>>,
-}
-
-pub(super) struct CustomEventInner<Request, Response, EventResponse, A, T>


I removed "inner" since we no longer need Mutex, we maintain a single copy and mutate it directly.

IvanGoncharov · 2025-04-16T14:11:29Z

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

                error!(http.method = "GET", "Hello from test");

-                let router_events = event_config.new_router_events();
+                let mut router_events = event_config.new_router_events();


Everything became explicitly mutable.
Previously it also was mutable but hidden under internal Arc + Mutex

IvanGoncharov · 2025-04-16T14:12:30Z

apollo-router/tests/integration/telemetry/fixtures/trace_id_via_header.router.yaml

@@ -1,9 +1,6 @@
 telemetry:
-


Was using this test as a basis for my new test and cleaned up some stuff in a process

IvanGoncharov · 2025-04-16T14:12:57Z

apollo-router/tests/integration/telemetry/propagation.rs

-        let _ = router.execute_query(Query::builder().body(json!({"query":"query {topProducts{name, name, name, name, name, name, name, name, name, name}}","variables":{}})).header("id_from_header".to_string(), trace_id.to_string()).build()).await;
+        let query = Query::builder()
+            .header("id_from_header".to_string(), trace_id.to_string())
+            .build();


we send default query here, just attach custom header to it

apollo-router/src/plugins/telemetry/config_new/connector/events.rs

bnjjj · 2025-04-17T09:00:59Z

apollo-router/src/plugins/telemetry/config_new/connector/events.rs

-                }),
-            }),
-        })
+        .filter_map(|(name, config)| CustomEvent::from_config(name, config))


Maybe we could still use TryFrom and convert it into an option using .ok()

bnjjj · 2025-04-17T09:05:07Z

apollo-router/src/plugins/telemetry/config_new/connector/events.rs

-                    return;
-                }
+    pub(crate) fn on_error(&mut self, error: &BoxError, ctx: &Context) {
+        if let Some(error_event) = &mut self.error {


Why don't we call .take() here like in request ? Just a question I'm not sure it's needed.

Technically, we can, if we sure it's the last call (which it probably is).
But I use take more as a hack to move event outside of this plugin.
In this case error event is handled inside so &mut is enough.

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

BrynCooke

Nice! It's great to have events standardized

IvanGoncharov · 2025-04-18T07:59:09Z

@mergify backport 1.x

mergify · 2025-04-18T07:59:15Z

backport 1.x

✅ Backports have been created

Details

#7313 Simplify implementation of telementry's events (backport #7280) has been created for branch 1.x but encountered conflicts

(cherry picked from commit e7d8e7b) # Conflicts: # apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap # apollo-router/src/plugins/connectors/handle_responses.rs # apollo-router/src/plugins/telemetry/config_new/connector/events.rs # apollo-router/src/plugins/telemetry/config_new/events.rs # apollo-router/src/plugins/telemetry/fmt_layer.rs # apollo-router/src/plugins/telemetry/mod.rs # apollo-router/src/services/connector/request_service.rs

router-perf · 2025-04-20T00:52:32Z

IvanGoncharov added 3 commits April 16, 2025 10:27

compiles after refactoring

1c9370c

remove mutexes in places where &mut is not required

156e581

cargo xtask dev

1675464

IvanGoncharov force-pushed the i1g/fix_events branch from d5785d3 to 1675464 Compare April 16, 2025 07:27

remove reference to a missing file

436f97a

IvanGoncharov changed the title ~~Check telemetry events on all stages of pipeline~~ Simplify implementation of telementry's events Apr 16, 2025

IvanGoncharov commented Apr 16, 2025

View reviewed changes

Add XXX comments

44a6e3d

IvanGoncharov commented Apr 16, 2025

View reviewed changes

apollo-router/src/plugins/telemetry/config_new/connector/events.rs Show resolved Hide resolved

IvanGoncharov commented Apr 16, 2025

View reviewed changes

remove FIXME

1f77164

IvanGoncharov commented Apr 16, 2025

View reviewed changes

apollo-router/src/plugins/telemetry/config_new/events.rs Show resolved Hide resolved

IvanGoncharov commented Apr 16, 2025

View reviewed changes

IvanGoncharov marked this pull request as ready for review April 16, 2025 14:13

IvanGoncharov requested review from a team, BrynCooke and bnjjj April 16, 2025 14:13

bnjjj reviewed Apr 17, 2025

View reviewed changes

BrynCooke approved these changes Apr 17, 2025

View reviewed changes

bnjjj approved these changes Apr 17, 2025

View reviewed changes

IvanGoncharov added 2 commits April 17, 2025 16:29

remove ecessive &mut

5c76457

Merge branch 'dev' into i1g/fix_events

16b3636

IvanGoncharov enabled auto-merge (squash) April 17, 2025 13:35

IvanGoncharov merged commit e7d8e7b into dev Apr 17, 2025
15 checks passed

IvanGoncharov deleted the i1g/fix_events branch April 17, 2025 13:48

IvanGoncharov mentioned this pull request Apr 18, 2025

Simplify implementation of telementry's events (backport #7280) #7313

Closed

6 tasks

IvanGoncharov mentioned this pull request Apr 20, 2025

Simplify condition handling in coprocessor #7320

Merged

6 tasks

	if self.request.level() != EventLevel::Off {
	if let Some(condition) = self.request.condition() {
	if condition.lock().evaluate_request(request) != Some(true) {
	return;

	if self.response.level() != EventLevel::Off {
	request
	.context
	.extensions()
	.with_lock(\|ext\| ext.insert(DisplayRouterResponse(true)));
	}
	for custom_event in &self.custom {
	custom_event.on_request(request);
	}

Conversation

IvanGoncharov commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

svc-apollo-docs commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Docs preview ready

Uh oh!

github-actions bot commented Apr 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrynCooke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IvanGoncharov commented Apr 18, 2025

Uh oh!

mergify bot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

router-perf bot commented Apr 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

IvanGoncharov commented Apr 16, 2025 •

edited

Loading

svc-apollo-docs commented Apr 16, 2025 •

edited

Loading

mergify bot commented Apr 18, 2025 •

edited

Loading