Skip to content

[ES][v2] Copy Jaeger<->OTLP translator from OTEL Contrib #6923

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
Merged
merged 10 commits into from
Mar 24, 2025
Merged

[ES][v2] Copy Jaeger<->OTLP translator from OTEL Contrib #6923

merged 10 commits into from
Mar 24, 2025

Conversation

Manik2708
Copy link
Contributor

Which problem is this PR solving?

Description of the changes

  • Implement the conversion of OTEL Traces to DB Spans as a first step for implementing v2 writer

How was this change tested?

  • Not tested yet

Checklist

@Manik2708 Manik2708 requested a review from a team as a code owner March 24, 2025 07:18
@Manik2708 Manik2708 requested a review from jkowall March 24, 2025 07:18
@Manik2708 Manik2708 marked this pull request as draft March 24, 2025 07:18
@Manik2708
Copy link
Contributor Author

Manik2708 commented Mar 24, 2025 •
edited
Loading

@yurishkuro @mahadzaryab1 Currently I am working on code-coverage for this converter, but before that I wanted to raise this PR to get a validation from your side to check if I am moving in right direction! Please can you give a review over this?

@codecov Codecov
Copy link

codecov bot commented Mar 24, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.05%. Comparing base (363f997) to head (83570bd).
Report is 1 commits behind head on main.

❌ Your project status has failed because the head coverage (30.05%) is below the target coverage (95.00%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (363f997) and HEAD (83570bd). Click for more details.

HEAD has 30 uploads less than BASE
Flag BASE (363f997) HEAD (83570bd)
badger_v1 2 0
tailsampling-processor 2 1
grpc_v1 2 0
badger_v2 2 0
unittests 2 0
memory_v2 2 0
kafka-3.x-v1 2 0
grpc_v2 2 0
kafka-3.x-v2 2 0
cassandra-5.x-v1-manual 2 0
opensearch-2.x-v1 2 0
elasticsearch-7.x-v1 2 0
elasticsearch-8.x-v2 2 0
elasticsearch-8.x-v1 2 1
cassandra-4.x-v2-auto 2 0
cassandra-4.x-v2-manual 2 0
Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #6923       +/-   ##
===========================================
- Coverage   96.13%   30.05%   -66.09%     
===========================================
  Files         340      178      -162     
  Lines       19608    10463     -9145     
===========================================
- Hits        18851     3145    -15706     
- Misses        572     7076     +6504     
- Partials      185      242       +57
Flag Coverage Δ
badger_v1 ?
badger_v2 ?
cassandra-4.x-v1-manual 15.36% <ø> (ø)
cassandra-4.x-v2-auto ?
cassandra-4.x-v2-manual ?
cassandra-5.x-v1-manual ?
cassandra-5.x-v2-auto 2.11% <ø> (ø)
cassandra-5.x-v2-manual 2.11% <ø> (ø)
elasticsearch-6.x-v1 20.12% <ø> (ø)
elasticsearch-7.x-v1 ?
elasticsearch-8.x-v1 20.38% <ø> (ø)
elasticsearch-8.x-v2 ?
grpc_v1 ?
grpc_v2 ?
kafka-3.x-v1 ?
kafka-3.x-v2 ?
memory_v2 ?
opensearch-1.x-v1 20.26% <ø> (ø)
opensearch-2.x-v1 ?
opensearch-2.x-v2 2.12% <ø> (ø)
tailsampling-processor 0.57% <ø> (ø)
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Manik2708
lint fix
55c2756 
Signed-off-by: Manik2708 <[email protected]>
yurishkuro
yurishkuro reviewed Mar 24, 2025
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

how did you come up with this code? I strongly recommend copying the code from OTEL and adjusting for the DB model, because the logical transformation is pretty much the same.

@yurishkuro
Copy link
Member

I would even say do a PR that is a straight copy (adjusting for package name), with tests and all, and then in the next PR adjust for dbmodel. This way we will see which changes are being applied to the otherwise official transformation.

@Manik2708
Copy link
Contributor Author

Yes, this code is highly inspired from the OTEL code!

@Manik2708 Manik2708 changed the title [ES][v2] Implement to_dbmodel to convert OTEL Traces to DB Spans [ES][v2] Copy jaegerproto_to_traces and traces_to_jaegerproto to Jaeger Mar 24, 2025
@Manik2708 Manik2708 marked this pull request as ready for review March 24, 2025 14:29
@yurishkuro yurishkuro added the changelog:exprimental Change to an experimental part of the code label Mar 24, 2025
@yurishkuro
Copy link
Member

please add tests to ensure 100% coverage, there's no reason for conversion code to be not covered.

@Manik2708
code-cov
22a07cd 
Signed-off-by: Manik2708 <[email protected]>
@Manik2708 Manik2708 requested a review from yurishkuro March 24, 2025 20:17
@Manik2708
header
83570bd 
Signed-off-by: Manik2708 <[email protected]>
@yurishkuro yurishkuro changed the title [ES][v2] Copy jaegerproto_to_traces and traces_to_jaegerproto to Jaeger [ES][v2] Copy Jaeger<->OTLP translator from OTEL Contrib Mar 24, 2025
@yurishkuro yurishkuro enabled auto-merge March 24, 2025 20:34
@yurishkuro yurishkuro disabled auto-merge March 24, 2025 20:53
@yurishkuro yurishkuro merged commit 8089924 into jaegertracing:main Mar 24, 2025
54 checks passed
@Manik2708 Manik2708 deleted the skeleton branch March 24, 2025 21:26
amilbcahat pushed a commit to amilbcahat/jaeger that referenced this pull request May 4, 2025
@Manik2708 @amol-verma-allen
[ES][v2] Copy Jaeger<->OTLP translator from OTEL Contrib (jaegertraci…
c2c1f21 
…ng#6923)

## Which problem is this PR solving?
- Fixes a part of: jaegertracing#6458 

## Description of the changes
- Implement the conversion of OTEL Traces to DB Spans as a first step
for implementing v2 writer

## How was this change tested?
- Not tested yet

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

---------

Signed-off-by: Manik2708 <[email protected]>
amilbcahat pushed a commit to amilbcahat/jaeger that referenced this pull request May 4, 2025
@Manik2708 @amol-verma-allen
[ES][v2] Copy Jaeger<->OTLP translator from OTEL Contrib (jaegertraci…
d3a2c68 
…ng#6923)

## Which problem is this PR solving?
- Fixes a part of: jaegertracing#6458

## Description of the changes
- Implement the conversion of OTEL Traces to DB Spans as a first step
for implementing v2 writer

## How was this change tested?
- Not tested yet

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

---------

Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: amol-verma-allen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@jkowall jkowall Awaiting requested review from jkowall jkowall is a code owner automatically assigned from jaegertracing/jaeger-maintainers

Assignees
No one assigned
Labels
area/storage changelog:exprimental Change to an experimental part of the code storage/elasticsearch v2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Manik2708 @yurishkuro