Skip to content

feat(opentelemetry source): Add JSON support #22963

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
Draft
fbs wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

feat(opentelemetry source): Add JSON support #22963

fbs wants to merge 11 commits into from

Conversation

@fbs
Copy link
Contributor

@fbs fbs commented Apr 29, 2025

This PR builds on #22957.

Once that is accepted I'll rebase this onto that. Opening this to show how the content-type handling from #22957 fits in.
This PR 'starts at' otel-proto: support json serialization

Summary

This adds JSON Protobuf encoding support to the open telemetry source. The PR has two main parts:

  1. adding JSON support to the types used. The implementation mostly stolen from opentelemetry-rust.
  2. selecting the right encoding for both request and reply based on the content-type header.

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

How did you test this PR?

Both sending actual logs as well as with test cases

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Notes

  • Please read our Vector contributor resources.
  • Do not hesitate to use @vectordotdev/vector to reach out to us regarding this PR.
  • The CI checks run only after we manually approve them.
    • We recommend adding a pre-push hook, please see this template.
    • Alternatively, we recommend running the following locally before pushing to the remote branch:
      • cargo fmt --all
      • cargo clippy --workspace --all-targets -- -D warnings
      • cargo nextest run --workspace (alternatively, you can run cargo test --all)
      • ./scripts/check_changelog_fragments.sh
  • After a review is requested, please avoid force pushes to help us review incrementally.
    • Feel free to push as many commits as you want. They will be squashed into one before merging.
    • For example, you can run git merge origin master and git push.
  • If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
    run cargo vdev build licenses to regenerate the license inventory and commit the changes (if any). More details here.

References

#22940

TODO

[] make sure traces and metrics have a test

@fbs fbs requested a review from a team as a code owner April 29, 2025 22:38
@github-actions github-actions bot added the domain: sources Anything related to the Vector's sources label Apr 29, 2025
fbs
fbs commented May 1, 2025
View reviewed changes
lib/opentelemetry-proto/build.rs Outdated
@@ -1,9 +1,100 @@
use std::io::Error;

// NOTE: Serde related attributes are copied from https://github.com/open-telemetry/opentelemetry-rust/
Copy link
Contributor Author

@fbs fbs May 1, 2025

Choose a reason for hiding this comment

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

Is this an issue wrt the CLA and licenses?

I initially started on an implementation and its ended up very similar (don't think tonic and serde offer an alternative way). Figured that copying it is easier as it makes it easier to sync if otel changes

Copy link
Contributor

@thomasqueirozb thomasqueirozb Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think that if this file is copied from the elsewhere then their license needs to be pasted in there

@pront pront self-assigned this May 8, 2025
@pront pront force-pushed the master branch 4 times, most recently from 1720078 to ffe54be Compare July 10, 2025 15:43
@pront
Copy link
Member

pront commented Jul 11, 2025

Hi @fbs, I was going over open PRs. Are you still motivated to contribute this?

@pront pront added the meta: awaiting author Pull requests that are awaiting their author. label Jul 11, 2025
@fbs
Copy link
Contributor Author

fbs commented Jul 12, 2025

hi @pront , yes sorry for the delay. Not a lot of spare time these days.

I think this is good to go in once #22957 is in. I'm updating that one now

fbs added 11 commits July 12, 2025 13:42
@fbs
add content-type extractor
2c04201 
This will allow us to support json in a later stage, as the content type
of the reply must match what the client used in the request.

This also improves error messages a bit, as its now a 415 instead of a
500. A 500 internal server error is quite confusing when debugging.
There is no error with the server, its the client sending the wrong
request.
@fbs
Add tests for new behaviour
689ea5c 
This tests the expected behaviour of the Open Telemetry source wrt error
handling. Currently vector mostly sends 500 Internal Server errors,
which isnt that helpful when debugging. It also means some retryable
errors are not retried.

The source implementation is done in the next step, so all tests fail.
@fbs
migrate from warp::rejection to reply
3beae3d 
Reject and recover indicates that the current filter didn't match but
that others might[1]. This means reject is not the appropriate tool for
handling e.g. deserialization errors.

This commit migrates from rejection to returning the reply directly, and
at the same time updates it to the new status codes to make sure it
passes the new tests.

This change also makes it possible to support `application/json` in the
future. The `content-type` of the reply must match that of the request,
and as we now have access to the request headers we can make sure they
match.

[1]: seanmonstar/warp#388 (comment)
@fbs
Avoid unwraps()
32f7729
@fbs
add changelog
22c3ed0
@fbs
clarify use of test case
44d42c7
@fbs
otel-proto: support json serialization
5d974c5 
Opentelemtry allows both binary protobuf as well as JSON protobuf when
using the HTTP transport. This commit adds support.

The implementation is basically stolen from opentelemetry-rust[1], so all
go credits to them.

[1]: https://github.com/open-telemetry/opentelemetry-rust
@fbs
google/rpc: Add json serialization
d3e284e 
This adds the ability to serialize, part of, Status to JSON.
The main usecase is to support Opentelemetry with HTTP/JSON and is thus
tailored to that.
As otel doesn't currently use 'detailed' that is skipped.
@fbs
add json support
9bb2876
@fbs
otlp: support both string and u64
9ed3d82 
Seems like both are allowed, so we need to support it
@fbs
add tests
68faf49
@github-actions github-actions bot removed the meta: awaiting author Pull requests that are awaiting their author. label Jul 12, 2025
@thomasqueirozb
Copy link
Contributor

thomasqueirozb commented Nov 17, 2025

Hi @fbs I'll make this a draft and we can re-open it once the dependent PR gets merged

@thomasqueirozb thomasqueirozb marked this pull request as draft November 17, 2025 19:34
@pront
Copy link
Member

pront commented Nov 17, 2025

Note that protobuf codecs now have new options and we also provide native OTLP support (otlp codecs).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomasqueirozb thomasqueirozb thomasqueirozb left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@pront pront

Labels

domain: sources Anything related to the Vector's sources

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fbs @pront @thomasqueirozb