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

Create metrics.Factory adapter for OTEL Metrics #5661

Merged
merged 52 commits into from
Jun 30, 2024
Merged

Create metrics.Factory adapter for OTEL Metrics #5661

merged 52 commits into from
Jun 30, 2024

Conversation

Wise-Wizard
Copy link
Contributor

Which problem is this PR solving?

This PR addresses a part of the issue #5633

Description of the changes
This is a Draft PR to bridge the OTEL Metrics instead of using Internal Metrics to minimize code changes.
How was this change tested?

The changes were tested by running the following command:

make test

Checklist

  • I have read CONTRIBUTING_GUIDELINES.md
  • I have signed all commits
  • I have added unit tests for the new functionality
  • I have run lint and test steps successfully
    • for jaeger: make lint test
    • for jaeger-ui: yarn lint and yarn test

@Wise-Wizard
Instantiated OTEL Metrics
f1f9b7a 
Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard Wise-Wizard requested a review from a team as a code owner June 19, 2024 17:17
@Wise-Wizard Wise-Wizard requested a review from joe-elliott June 19, 2024 17:17
@Wise-Wizard Wise-Wizard marked this pull request as draft June 19, 2024 17:17
@codecov Codecov
Copy link

codecov bot commented Jun 19, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.38%. Comparing base (44484ac) to head (3265bc0).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #5661   +/-   ##
=======================================
  Coverage   96.38%   96.38%           
=======================================
  Files         329      334    +5     
  Lines       16060    16141   +81     
=======================================
+ Hits        15479    15558   +79     
- Misses        404      405    +1     
- Partials      177      178    +1
Flag Coverage Δ
badger_v1 8.04% <ø> (ø)
badger_v2 1.92% <ø> (ø)
cassandra-3.x-v1 16.60% <ø> (ø)
cassandra-3.x-v2 1.84% <ø> (ø)
cassandra-4.x-v1 16.60% <ø> (ø)
cassandra-4.x-v2 1.84% <ø> (ø)
elasticsearch-7.x-v1 18.87% <ø> (ø)
elasticsearch-8.x-v1 19.07% <ø> (-0.02%) ⬇️
elasticsearch-8.x-v2 19.08% <ø> (ø)
grpc_v1 9.46% <ø> (-0.02%) ⬇️
grpc_v2 7.49% <ø> (ø)
kafka 9.76% <ø> (ø)
opensearch-1.x-v1 18.93% <ø> (+0.01%) ⬆️
opensearch-2.x-v1 18.93% <ø> (ø)
opensearch-2.x-v2 18.92% <ø> (ø)
unittests 94.25% <100.00%> (+0.01%) ⬆️

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.

@Wise-Wizard
Ran make fmt
12403b1 
Signed-off-by: Wise-Wizard <[email protected]>
yurishkuro
yurishkuro reviewed Jun 19, 2024
View reviewed changes
pkg/metrics/factory.go Outdated Show resolved Hide resolved
pkg/metrics/otelCounter.go Outdated Show resolved Hide resolved
yurishkuro
yurishkuro reviewed Jun 21, 2024
View reviewed changes
pkg/metrics/otelmetrics/factory.go Outdated Show resolved Hide resolved
pkg/metrics/otelmetrics/otelCounter.go Outdated Show resolved Hide resolved
pkg/metrics/otelmetrics/factory.go Outdated Show resolved Hide resolved
pkg/metrics/otelmetrics/otelCounter.go Outdated Show resolved Hide resolved
@Wise-Wizard Wise-Wizard requested a review from yurishkuro June 22, 2024 19:00
Wise-Wizard and others added 2 commits June 23, 2024 01:32
@Wise-Wizard
Merge branch 'main' into OTEL_Metrics
f508b8a
@yurishkuro
avoid allocations
4a99291 
tested via
```
$ go test -benchmem -benchtime=5s -bench=Benchmark ./internal/metrics/
```

before:
```
BenchmarkPrometheusCounter-10           856818336                6.875 ns/op           0 B/op          0 allocs/op
BenchmarkOTELCounter-10                 146044255               40.92 ns/op           32 B/op          2 allocs/op
```

after:
``
BenchmarkPrometheusCounter-10           855046669                6.924 ns/op           0 B/op          0 allocs/op
BenchmarkOTELCounter-10                 293330721               21.05 ns/op           16 B/op          1 allocs/op
```

Signed-off-by: Yuri Shkuro <[email protected]>
yurishkuro
yurishkuro reviewed Jun 22, 2024
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.

run make fmt to add headers

internal/metrics/otelmetrics/factory.go Outdated Show resolved Hide resolved
internal/metrics/otelmetrics/otelCounter.go Outdated Show resolved Hide resolved
@yurishkuro
Copy link
Member

I fixed perf by 2x in 4a99291. For some reason OTEL counter still does a memory allocation for each Inc() call. We should open an upstream ticket for that.

@Wise-Wizard
Copy link
Contributor Author

I fixed perf by 2x in 4a99291. For some reason OTEL counter still does a memory allocation for each Inc() call. We should open an upstream ticket for that.

Oh that's great!

@Wise-Wizard
Implemented changes
099ba05 
Signed-off-by: Wise-Wizard <[email protected]>
@yurishkuro
Copy link
Member

I fixed perf by 2x in 4a99291

Actually, your benchmark is incorrect. You are not initializing OTEL SDK, so the benchmark is actually going to noop implementation (even more surprising why it performs an allocation in no-op mode).

@Wise-Wizard
Copy link
Contributor Author

Wise-Wizard commented Jun 23, 2024 •
edited
Loading

I fixed perf by 2x in 4a99291

Actually, your benchmark is incorrect. You are not initializing OTEL SDK, so the benchmark is actually going to noop implementation (even more surprising why it performs an allocation in no-op mode).

Yes I do realise that, I wrote the benchmark first and just ran it with no-op implementation for the looks of it expecting a very low time to be taken.
But, the no-op implementation itself had a very poor performance, so I thought I would raise it first, and didn't bother to check with the correct implementation

@yurishkuro
Copy link
Member

I am quite sure that the perf hit is coming from that one still remaining allocation.

@Wise-Wizard
Added Go Leak Test
7deeb6b 
Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard Wise-Wizard marked this pull request as ready for review June 27, 2024 15:08
@Wise-Wizard
Fixed Alias Error
92ea1bb 
Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard @yurishkuro
Update internal/metrics/otelmetrics/factory_test.go
47c2aed 
Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: Saransh Shankar <[email protected]>
@Wise-Wizard
Implemented missing Tests
36dda89 
Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard
Created Table Driven Test
d4ee6e0 
Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard
Added invalid test for all metrics
5e7c624 
Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard
Added check for Empty Namespace
965ad84 
Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard
Disabled OTEL Scope Info
43de5d1 
Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard
Test Nested Namespace
878e5f4 
Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard
Ran make Lint
864acde 
Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard
Ran make fmt
0054c5c 
Signed-off-by: Wise-Wizard <[email protected]>
@yurishkuro yurishkuro changed the title Instantiated OTEL Metrics Create metrics.Factory adapter for OTEL Metrics Jun 30, 2024
@yurishkuro yurishkuro merged commit 301dbec into jaegertracing:main Jun 30, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro left review comments

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

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

Successfully merging this pull request may close these issues.
2 participants
@Wise-Wizard @yurishkuro