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.

Sign up for GitHub

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 for custom attributes for spans and logs #4102

Merged
merged 109 commits into from
Nov 21, 2023
Merged

Add support for custom attributes for spans and logs #4102

merged 109 commits into from
Nov 21, 2023

Conversation

bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Oct 26, 2023 •
edited
Loading

Fixes #4096
Fixes #4086
Fixes #3543
Fixes #3537

Checklist

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

  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Tests added and passing3
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

Footnotes

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

  2. Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩

  3. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

bnjjj added 3 commits October 25, 2023 17:41
@bnjjj
wip
c3919d5 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
custom dynamic attributes is working for logs and spans
4b4f703 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
wip
2d7c51c 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@github-actions
Copy link
Contributor

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

@router-perf
Copy link

router-perf bot commented Oct 26, 2023 •
edited by bnjjj
Loading

CI performance tests

  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • large-request - Stress test with a 1 MB request payload
  • step - Basic stress test that steps up the number of users over time
  • xlarge-request - Stress test with 10 MB request payload
  • reload - Reload test over a long period of time at a constant rate of users
  • no-graphos - Basic stress test, no GraphOS.
  • xxlarge-request - Stress test with 100 MB request payload
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • const - Basic stress test that runs with a constant number of users

bnjjj and others added 14 commits October 26, 2023 17:31
@bnjjj
add support of custom attributes in json formatter
0903984 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
wip
e69c986 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
Attributes workaround
639c3e7
@bnjjj
supergraph done, missing subgraph
676e4e4 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
add support of custom attributes for subgraph
3e9a77b 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
add better implementation of dynamic attributes with sampling
bbdc0e8 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
fix lint
c68a14f 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
Merge branch 'dev' of github.com:apollographql/router into bnjjj/cust…
d39f85c 
…om_attributes_telemetry
@bnjjj
fix tests
df02c43 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
comment dyn attribute in text formatter
448d8b8 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
use otel constants
9def3e3 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
add support of requirement level
cb1d523 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
lint
3f94291 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
address review comments
3874345 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj marked this pull request as ready for review November 2, 2023 16:32
@bnjjj bnjjj requested a review from a team as a code owner November 2, 2023 16:32
bnjjj and others added 24 commits November 10, 2023 14:56
@bnjjj
fix test
746e1e7 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
Merge branch 'bnjjj/custom_attributes_telemetry' of github.com:apollo…
7d69ab9 
…graphql/router into bnjjj/custom_attributes_telemetry
Rename Trace to TracingCommon for consistency
1bcb533
@bnjjj
update snapshots
32b1dbe 
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
Merge branch 'bnjjj/custom_attributes_telemetry' of github.com:apollo…
1a4498e 
…graphql/router into bnjjj/custom_attributes_telemetry
Rename new mode to spec_compliant
5d17679
Add resource support to logging.
5af8456
Migrate telemetry.(logging|tracing|metrics) to telemetry.exporters.(l…
fb00e16 
…ogging|tracing|metrics)
Change name of GetAttribute and GetAttributes to Selector and Selectors.
5832540
Add missing snapshot
e7f0948
temp
8c5795b
Lint
af7defd
Conditions tests
1ad350b
Fix tests
961a9d9
Finish tests for conditions.
478c8af
@bnjjj
telemetry: get rid of the tracing_fmt layer and use our own optimized…
266db72 
… one (#4202)

Some perf improvements to lazy parse the attribute for logs

---------

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Remove many of the dead_code statements.
cee76f4 
Fix compile errors.
Move some attribute stuff to the correct files in prep for testing.
Add tests for attributes, fix bugs found once tests were added.
d92a13f
@bnjjj
telemetry: add tests for the formatting layer (#4209)
4612e00 
add tests for the formatting layer

---------

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Move some code around for defaults_for_level.
9572725 
Add tests for `RouterSpans`, `SupergraphSpans` and `SubgraphSpans`.
Add more tests for span attributes.
4dab1b0
@bnjjj
Merge branch 'dev' of github.com:apollographql/router into bnjjj/cust…
103f575 
…om_attributes_telemetry
@bnjjj
Merge branch 'bnjjj/custom_attributes_telemetry' of github.com:apollo…
9c222e2 
…graphql/router into bnjjj/custom_attributes_telemetry
Fix dev mode
18b4165
@bnjjj bnjjj merged commit 3fa7c11 into dev Nov 21, 2023
11 of 12 checks passed
@bnjjj bnjjj deleted the bnjjj/custom_attributes_telemetry branch November 21, 2023 09:55
@BrynCooke BrynCooke mentioned this pull request Nov 23, 2023
Merged
6 tasks
@bnjjj bnjjj mentioned this pull request Nov 27, 2023
Closed
Geal pushed a commit that referenced this pull request Nov 29, 2023
@BrynCooke
Make Jaeger tests run without APOLLO_KEY and APOLLO_GRAPH_REF (#4261)
856ccda 
#4102 Introduced some changes to tests that would result in failures if
the developer did not have an enterprise account. This is now gated on
the appropriate env variables being present.

Changes:
Revert configuration of existing jaeger tests for not use custom span
attributes.
Add a new test specifically for this.

<!-- start metadata -->
---

**Checklist**

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

- [ ] Changes are compatible[^1]
- [ ] Documentation[^2] completed
- [ ] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [ ] Integration Tests
    - [ ] Manual Tests

**Exceptions**

*Note any exceptions here*

**Notes**

[^1]: 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.
[^2]: Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]: Tick whichever testing boxes are applicable. If you are adding
Manual Tests, please document the manual testing (extensively) in the
Exceptions.

Co-authored-by: bryn <bryn@apollographql.com>
@Geal Geal mentioned this pull request Jan 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Geal Geal Geal approved these changes

@garypen garypen garypen left review comments

@o0Ignition0o o0Ignition0o o0Ignition0o approved these changes

@lennyburdette lennyburdette Awaiting requested review from lennyburdette

@BrynCooke BrynCooke Awaiting requested review from BrynCooke

Assignees

@bnjjj bnjjj

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@bnjjj @garypen @Geal @BrynCooke @o0Ignition0o