WIP: experiment to support Apollo tracing over OTLP #4982

timbotnik · 2024-04-18T13:36:36Z

Very early in the process here. Just trying to get some feedback on the approach and figure out why there is some complaints about a threading issue 👀

Fixes #issue_number

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. ↩

github-actions · 2024-04-18T13:36:51Z

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

router-perf · 2024-04-18T13:46:10Z

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

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

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

bnjjj · 2024-04-18T13:44:31Z

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

+// TBD(tim): maybe we should move this?
+// Also, maybe we can just use the actual SpanData instead of the light one?


The main idea of having this LightSpanData is to consume less memory in the LRU cache. it was a good improvement when introduced

Yeah another approach would be grouping by trace and attributes to get rid of the duplications, which would be nice for the wire protocol as well. Probably something to visit in the next phase when we feel good about moving entirely to OTLP.

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

…tudio

Note that this is very much unvetted and doesn’t quite compile yet.

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>

…d of Arc

…initial-support

bonnici · 2024-04-24T06:51:15Z

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

+    // Use only the Apollo usage reporting protobuf over http
+    Apollo,
+    // Use both the Apollo usage reporting protobuf AND Apollo OTLP
+    ApolloAndOtlp,


Minor but there's a pretty common pattern here of using New/Legacy/Both as the enum names (though maybe we should use Otlp/Apollo/Both instead?) https://docs.google.com/document/d/1Zk6PJyCwgDIMTRBSt3UoCKPfefM25saKCDn_AtEHQnc/edit#heading=h.am5dyhwon4gw

Ah cool - happy to use a convention if there is one established 👍

Also could you change testing by experimental to be consistent with what we currently have

…porter Which with my current skills means we’ll have to clone all of the spans. Possibly there is a way to use shared span pointers here instead but I’ll leave that for an optimization pass.

…es of LightSpanData’s

… cache

bnjjj

Missing integration and unit tests and don't use unwrap, if you're 100% sure it's ok to unwrap then use expect() instead with your message inside explaining why it's safe to do this

bnjjj · 2024-05-14T09:20:20Z

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

+const REPORTS_INCLUDE_ATTRS: [Key; 17] = [
+    APOLLO_PRIVATE_REQUEST,
+    APOLLO_PRIVATE_DURATION_NS_KEY,
+    APOLLO_PRIVATE_SENT_TIME_OFFSET,
+    APOLLO_PRIVATE_GRAPHQL_VARIABLES,
+    APOLLO_PRIVATE_HTTP_REQUEST_HEADERS,
+    APOLLO_PRIVATE_HTTP_RESPONSE_HEADERS,
+    APOLLO_PRIVATE_OPERATION_SIGNATURE,
+    APOLLO_PRIVATE_FTV1,
+    PATH,
+    SUBGRAPH_NAME,
+    CLIENT_NAME_KEY,
+    CLIENT_VERSION_KEY,
+    DEPENDS,
+    LABEL,
+    CONDITION,
+    OPERATION_NAME,
+    OPERATION_TYPE,
+];


For this make sure to use an HashSet at the end because perfs are better when you're checking if it contains an element. Here is an example here

Oh I think you already did it. Just make sure it's done that way

bnjjj · 2024-05-14T09:43:16Z

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

+    // Use only the Apollo usage reporting protobuf over http
+    Apollo,
+    // Use both the Apollo usage reporting protobuf AND Apollo OTLP
+    ApolloAndOtlp,


Also could you change testing by experimental to be consistent with what we currently have

bnjjj · 2024-05-14T09:44:30Z

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

+            // TBD(tim): do we need another batch processor for this?
+            // Seems like we've already set up a batcher earlier in the pipe but not quite sure.


cc @BrynCooke you might be the best one to answer this

bnjjj · 2024-05-14T09:46:39Z

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

+    resource_template: Resource,
+    intrumentation_library: InstrumentationLibrary,
+    #[derivative(Debug = "ignore")]
+    otlp_exporter: Arc<Mutex<opentelemetry_otlp::SpanExporter>>,


Could you use https://github.com/Amanieu/parking_lot mutex ? It's already in our deps and usually more perfs

bnjjj · 2024-05-14T09:48:10Z

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

+        exporter.export(spans)
+    }
+
+    pub(crate) fn shutdown(&self) {


If it's a shutdown and we don't use the exporter itself after maybe we should take ownership ? Just a suggestion I don't know if it's valid

Suggested change

pub(crate) fn shutdown(&self) {

pub(crate) fn shutdown(self) {

bnjjj · 2024-05-14T09:48:31Z

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

+];
+
+/// Additional attributes to include when sending to the OTLP protocol.
+const OTLP_EXT_INCLUDE_ATTRS: [Key; 6] = [


same for hashset

bnjjj · 2024-05-14T09:50:21Z

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

+                // https://github.com/open-telemetry/opentelemetry-rust/blob/943bb7a03f9cd17a0b6b53c2eb12acf77764c122/opentelemetry-sdk/CHANGELOG.md?plain=1#L157-L159
+                let max_attr_len = std::cmp::min(attr_names.len(), value.attributes.len());
+                let mut new_attrs = EvictedHashMap::new(max_attr_len.try_into().unwrap(), max_attr_len);
+                value.attributes.iter().for_each(|(key, value)| {


If you want to avoid copies then use into_iter() instead and then in the loop you'll be able to delete the clone

Suggested change

value.attributes.iter().for_each(|(key, value)| {

value.attributes.into_iter().for_each(|(key, value)| {

bnjjj · 2024-05-14T09:52:46Z

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

@@ -229,6 +321,13 @@ impl Exporter {
            errors_configuration: errors_configuration.clone(),
            use_legacy_request_span: use_legacy_request_span.unwrap_or_default(),
            include_span_names: INCLUDE_SPANS.into(),
+            include_attr_names: match apollo_tracing_protocol {
+                // TBD(tim): consider filtering the current implementation for memory purposes to HashSet::from(REPORTS_INCLUDE_ATTRS)


I think you can remove this comment

bnjjj · 2024-05-14T09:53:13Z

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

@@ -322,6 +421,63 @@ impl Exporter {
        Ok(results)
    }

+    fn init_spans_for_tree(&self, root_span: &LightSpanData) -> Vec<SpanData> {
+        let exporter = self.otlp_exporter.as_ref().unwrap();


We should not use unwrap

bnjjj · 2024-05-14T09:57:01Z

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

+
+    fn shutdown(&mut self) {
+        // currently only handled in the OTLP case
+        if let Some(exporter) = self.otlp_exporter.clone() {


Suggested change

if let Some(exporter) = self.otlp_exporter.clone() {

if let Some(exporter) = &self.otlp_exporter {

or you can .take() it if you don't need it anymore ?

timbotnik changed the title ~~Timbotnik/apollo otlp/initial support~~ WIP: initial support for Apollo over OTLP Apr 18, 2024

timbotnik changed the title ~~WIP: initial support for Apollo over OTLP~~ WIP: experiment to support for Apollo over OTLP Apr 18, 2024

timbotnik changed the title ~~WIP: experiment to support for Apollo over OTLP~~ WIP: experiment to support Apollo tracing over OTLP Apr 18, 2024

timbotnik requested a review from bnjjj April 18, 2024 13:38

timbotnik commented Apr 18, 2024

View reviewed changes

apollo-router/src/plugins/telemetry/apollo_otlp_exporter.rs Outdated Show resolved Hide resolved

timbotnik commented Apr 18, 2024

View reviewed changes

apollo-router/src/plugins/telemetry/tracing/apollo_telemetry.rs Outdated Show resolved Hide resolved

bnjjj reviewed Apr 18, 2024

View reviewed changes

timbotnik added 3 commits April 19, 2024 01:32

Add a new configuration for setting the tracing protocol for Apollo S…

5cf5750

…tudio

Modify some exports that we’ll need

d1024dc

Add the OTLP path

360f8fb

Note that this is very much unvetted and doesn’t quite compile yet.

timbotnik force-pushed the timbotnik/apollo-otlp/initial-support branch from ea8c4a2 to b0755b2 Compare April 18, 2024 15:32

Fix up some loose ends, review notes

376f480

timbotnik force-pushed the timbotnik/apollo-otlp/initial-support branch from b0755b2 to 376f480 Compare April 18, 2024 15:41

bnjjj and others added 6 commits April 19, 2024 10:21

fixes

f4973e4

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>

Turn the ApollOtlpExporter into a SpanExporter, try direct ref instea…

2d5c59b

…d of Arc

Use interior mutability with Arcs to improve lifetimes

738e2b3

Merge remote-tracking branch 'origin/dev' into timbotnik/apollo-otlp/…

0728090

…initial-support

Add shutdown handling

e35b241

Code cleanup

2004a6e

bonnici reviewed Apr 24, 2024

View reviewed changes

timbotnik added 8 commits April 24, 2024 23:13

Fix an issue where we’d be stealing the spans away from the Apollo ex…

0730668

…porter Which with my current skills means we’ll have to clone all of the spans. Possibly there is a way to use shared span pointers here instead but I’ll leave that for an optimization pass.

Prepare SpanData’s during collection, prevents making additional copi…

2ce4d1a

…es of LightSpanData’s

Introduce “OTLP” only option, refactor to support peek vs. pop on the…

9a5fa87

… cache

Run cargo fmt

51c43bf

Run xtask lint —fmt

e7ac75d

Manual lint fixes

6b08eb8

Clippy is sometimes wrong

c418e2a

Stop filtering spans for now, can revisit this later.

02246e8

timbotnik added 3 commits April 25, 2024 21:57

Turn off compression temporarily till we enable on the collector

8837262

Move ROUTER_ID to a higher order crate

c487ffa

Add attribute-level filtering support for OTel spans.

cd3328f

bnjjj reviewed May 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: experiment to support Apollo tracing over OTLP #4982

WIP: experiment to support Apollo tracing over OTLP #4982

timbotnik commented Apr 18, 2024 •

edited

github-actions bot commented Apr 18, 2024

router-perf bot commented Apr 18, 2024

bnjjj Apr 18, 2024

timbotnik Apr 18, 2024

bonnici Apr 24, 2024

timbotnik Apr 25, 2024

bnjjj May 14, 2024

bnjjj left a comment

bnjjj May 14, 2024

bnjjj May 14, 2024

bnjjj May 14, 2024

bnjjj May 14, 2024

bnjjj May 14, 2024

bnjjj May 14, 2024

bnjjj May 14, 2024

bnjjj May 14, 2024

bnjjj May 14, 2024

bnjjj May 14, 2024

bnjjj May 14, 2024

		// TBD(tim): maybe we should move this?
		// Also, maybe we can just use the actual SpanData instead of the light one?

		// TBD(tim): do we need another batch processor for this?
		// Seems like we've already set up a batcher earlier in the pipe but not quite sure.

	pub(crate) fn shutdown(&self) {
	pub(crate) fn shutdown(self) {

	value.attributes.iter().for_each(\|(key, value)\| {
	value.attributes.into_iter().for_each(\|(key, value)\| {

	if let Some(exporter) = self.otlp_exporter.clone() {
	if let Some(exporter) = &self.otlp_exporter {

WIP: experiment to support Apollo tracing over OTLP #4982

Are you sure you want to change the base?

WIP: experiment to support Apollo tracing over OTLP #4982

Conversation

timbotnik commented Apr 18, 2024 • edited

Footnotes

github-actions bot commented Apr 18, 2024

router-perf bot commented Apr 18, 2024

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

bnjjj 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

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

timbotnik commented Apr 18, 2024 •

edited