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.

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

Implement adjusters to operate on OTLP data format #6344

Closed
13 tasks done
mahadzaryab1 opened this issue Dec 12, 2024 · 7 comments
Closed
13 tasks done

Implement adjusters to operate on OTLP data format #6344

mahadzaryab1 opened this issue Dec 12, 2024 · 7 comments

Comments

@mahadzaryab1
Copy link
Collaborator

mahadzaryab1 commented Dec 12, 2024

Today, we have adjusters in model/adjuster that perform modifications to a trace object. We want to implement v2 versions of all these adjusters that operate directly on the OTLP data format (ptrace.traces instead of on model.Trace).

@mahadzaryab1 mahadzaryab1 self-assigned this Dec 12, 2024
yurishkuro pushed a commit that referenced this issue Dec 12, 2024
…#6346)

## Which problem is this PR solving?
- Towards #6344

## Description of the changes
- This PR adds an interface for `Adjuster` to operate on the OTLP model
format so that it can be used by the v2 query service. The v1
interface/implementation can be found in `model/adjuster`.
- In the following PRs, we'll implement all the standard adjusters in
`model/adjuster` to implement the new interface.

## How was this change tested?
- Unit tests

## 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`: `yarn lint` and `yarn test`

---------

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1 added a commit that referenced this issue Dec 13, 2024
…del (#6354)

## Which problem is this PR solving?
- Towards #6344 

## Description of the changes
- This PR implements the `SpanReferences` adjuster to operate on the
OTLP data model. In the OTLP model, references are dubbed as links so
the adjuster was renamed to `SpanLinks`

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
yurishkuro pushed a commit that referenced this issue Dec 13, 2024
…a model (#6355)

## Which problem is this PR solving?
- Towards #6344

## Description of the changes
- This PR implements the `IPTag` adjuster to operate on the OTLP data
model. In the OTLP model, tags are dubbed as attributes so the adjuster
was renamed to `IPAttribute`.

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1
Copy link
Collaborator Author

The parent reference adjuster is not needed when operating on the OTLP data model because because the parent span ID is recorded explicitly on the span instead of a reference (or a link in OTLP terminology).

yurishkuro pushed a commit that referenced this issue Dec 14, 2024
…ata model (#6358)

## Which problem is this PR solving?
- Towards #6344

## Description of the changes
- This PR implements the `OTelTag` adjuster to operate on the OTLP data
model. In the OTLP model, tags are dubbed as attributes so the adjuster
was renamed to `OTELAttribute`.

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
@yurishkuro
Copy link
Member

Some adjusters cannot be implemented in OTLP in the same way. For example, pdata.Traces has no place to store warnings, and the sorting adjuster won't work because pdata uses maps internally for attributes. Given that the UI isn't going to accept OTLP anytime soon, even if the internals and APIs like APIv3 all support OTLP we would still need a transformation into UI JSON data model, which is easiest to achieve today with OTLP -> model -> uijson. It means that whichever adjusters we are not fully able to do in the OTLP layer would need to be injected into OTLP -> model. I would suggest we replace all instances of OTLP -> model conversions we might have in the code with a call to a helper function in the jotlp package (which on reflection we should rename jpdata).

@yurishkuro
Copy link
Member

Also, we might already have some data loss in the factoryadapter.TraceReader because storage backends are expected to store span warnings that might be produced by inbound sanitizers.

@mahadzaryab1
Copy link
Collaborator Author

Also, we might already have some data loss in the factoryadapter.TraceReader because storage backends are expected to store span warnings that might be produced by inbound sanitizers.

@yurishkuro I don't follow this point. Would you be able to elaborate? How do we have data loss?

@yurishkuro
Copy link
Member

How do we have data loss?

Suppose we utilize Span.Warnings on the ingestion path. It gets stored in the database, and then v1 reader re-inflates it back into Span.Warnings. But then when we wrap that reader into adapter reader, we call model2pdata from OTEL, which may ignore Span.Warnings and they get lost. We can check how that transform is implemented and perhaps fix it to use the same jaeger.internal.warnings attribute when translating from the model.

@mahadzaryab1
Copy link
Collaborator Author

mahadzaryab1 commented Dec 16, 2024

How do we have data loss?

Suppose we utilize Span.Warnings on the ingestion path. It gets stored in the database, and then v1 reader re-inflates it back into Span.Warnings. But then when we wrap that reader into adapter reader, we call model2pdata from OTEL, which may ignore Span.Warnings and they get lost. We can check how that transform is implemented and perhaps fix it to use the same jaeger.internal.warnings attribute when translating from the model.

Got it. We can fix this by adding a wrapper around the translator in jptrace like you suggested. There's already an item on the checklist for that.

mahadzaryab1 added a commit that referenced this issue Dec 16, 2024
…turn implemented struct (#6362)

## Which problem is this PR solving?
- Towards #6344 

## Description of the changes
- This PR performs the following refactorings to the adjuster package
  - Remove the `Func` alias 
- Change the implemented adjusters to return a struct that implements
the Adjuster interface
- Change the interface to only return an error to indicate that traces
are modified in place
  - Move the warnings utility to `cmd/query/app/internal/jotlp`

## How was this change tested?
- CI and unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1 added a commit that referenced this issue Dec 18, 2024
## Which problem is this PR solving?
- Towards #6344 

## Description of the changes
- Add a warning to the `SpanLinks` adjuster when there is a bad span
link to match the behaviour of the v1 adjuster
(https://github.com/jaegertracing/jaeger/blob/main/model/adjuster/bad_span_references.go#L40)

## How was this change tested?
- Updated unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1 added a commit that referenced this issue Dec 18, 2024
…lp data model (#6367)

## Which problem is this PR solving?
- Towards #6344 

## Description of the changes
- Implement the Span ID Uniquifier adjuster to operate on the OTLP data
model.

## How was this change tested?
- Unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
yurishkuro pushed a commit that referenced this issue Dec 18, 2024
## Which problem is this PR solving?
- Towards #6344

## Description of the changes
- This PR removes the `error` return value from the v2 adjuster
interface as none of the adjusters return any errors and errors instead
recorded on the spans as warnings.

## How was this change tested?
- CI

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1 added a commit that referenced this issue Dec 20, 2024
…6389)

## Which problem is this PR solving?
- Towards #6344

## Description of the changes
- Implemented an adjuster to sort attributes and events that operates
natively on the OTLP data format. This is analogous to the
`SortTagsAndLogFields` adjuster in v1.

## How was this change tested?
- Added a unit test

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1 added a commit that referenced this issue Dec 22, 2024
## Which problem is this PR solving?
- Towards #6344

## Description of the changes
- Implemented an adjuster to deduplicate spans. 
- The span deduplication is done by marshalling each span into protobuf
bytes and applying the FNV hash algorithm to it.

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1 added a commit that referenced this issue Dec 23, 2024
#6392)

## Which problem is this PR solving?
- Towards #6344

## Description of the changes
- Implemented an adjuster to correct timestamps for clockskew. 

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
ekefan pushed a commit to ekefan/jaeger that referenced this issue Dec 23, 2024
…ing#6391)

## Which problem is this PR solving?
- Towards jaegertracing#6344

## Description of the changes
- Implemented an adjuster to deduplicate spans. 
- The span deduplication is done by marshalling each span into protobuf
bytes and applying the FNV hash algorithm to it.

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
ekefan pushed a commit to ekefan/jaeger that referenced this issue Dec 23, 2024
jaegertracing#6392)

## Which problem is this PR solving?
- Towards jaegertracing#6344

## Description of the changes
- Implemented an adjuster to correct timestamps for clockskew. 

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1 added a commit that referenced this issue Dec 23, 2024
#6394)

## Which problem is this PR solving?
- Towards #6344

## Description of the changes
- Implemented a translator in `jptrace` with a function
`ProtoFromTraces` that is a wrapper of the upstream `ProtoFromTraces` in
opentelemetry-collector-contrib jaeger translator. This function appends
the warnings in `jaeger.internal.warnings` to the corresponding warnings
field in the proto model.

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1 added a commit that referenced this issue Dec 23, 2024
…e on otlp format (#6396)

## Which problem is this PR solving?
- Towards #6344 

## Description of the changes
- Implemented a function `StandardAdjusters` that returns a list of
adjusters to be applied on ptrace.Traces
- This will be used by the v2 query service in
#6343

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
yurishkuro pushed a commit that referenced this issue Dec 24, 2024
#6397)

## Which problem is this PR solving?
- Towards #6344

## Description of the changes
- Implemented a translator in `jptrace` with a function `ProtoToTraces`
that is a wrapper of the upstream `ProtoToTraces` in
opentelemetry-collector-contrib jaeger translator. This function appends
the warnings in model.Span to the `jaeger.internal.warnings` attribute
in the corresponding span in ptrace.Traces.

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1
Copy link
Collaborator Author

mahadzaryab1 commented Dec 25, 2024

All adjusters have been migrated and the development of the v2 query service is now unblocked

Manik2708 pushed a commit to Manik2708/jaeger that referenced this issue Jan 5, 2025
jaegertracing#6392)

## Which problem is this PR solving?
- Towards jaegertracing#6344

## Description of the changes
- Implemented an adjuster to correct timestamps for clockskew. 

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
Manik2708 pushed a commit to Manik2708/jaeger that referenced this issue Jan 5, 2025
jaegertracing#6394)

## Which problem is this PR solving?
- Towards jaegertracing#6344

## Description of the changes
- Implemented a translator in `jptrace` with a function
`ProtoFromTraces` that is a wrapper of the upstream `ProtoFromTraces` in
opentelemetry-collector-contrib jaeger translator. This function appends
the warnings in `jaeger.internal.warnings` to the corresponding warnings
field in the proto model.

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
Manik2708 pushed a commit to Manik2708/jaeger that referenced this issue Jan 5, 2025
…e on otlp format (jaegertracing#6396)

## Which problem is this PR solving?
- Towards jaegertracing#6344 

## Description of the changes
- Implemented a function `StandardAdjusters` that returns a list of
adjusters to be applied on ptrace.Traces
- This will be used by the v2 query service in
jaegertracing#6343

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
Manik2708 pushed a commit to Manik2708/jaeger that referenced this issue Jan 5, 2025
jaegertracing#6397)

## Which problem is this PR solving?
- Towards jaegertracing#6344

## Description of the changes
- Implemented a translator in `jptrace` with a function `ProtoToTraces`
that is a wrapper of the upstream `ProtoToTraces` in
opentelemetry-collector-contrib jaeger translator. This function appends
the warnings in model.Span to the `jaeger.internal.warnings` attribute
in the corresponding span in ptrace.Traces.

## How was this change tested?
- Added unit tests

## 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: Mahad Zaryab <mahadzaryab1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants