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

Fix duplicate logs across resources #5803

Merged
merged 25 commits into from
Sep 17, 2024
Merged

Fix duplicate logs across resources #5803

merged 25 commits into from
Sep 17, 2024

Conversation

pree-dew
Copy link
Contributor

@pree-dew pree-dew commented Sep 10, 2024 •
edited
Loading

  1. Create scope map with resource key to map the correct log record.
  2. Add test case with different resource and scope combination

Fixes #5782

Benchmarks

goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/internal/transform
               │   old.txt   │               new.txt               │
               │   sec/op    │   sec/op     vs base                │
ResourceLogs-8   3.266µ ± 3%   1.100µ ± 5%  -66.33% (p=0.000 n=10)

               │   old.txt    │               new.txt                │
               │     B/op     │     B/op      vs base                │
ResourceLogs-8   8.297Ki ± 0%   2.430Ki ± 0%  -70.72% (p=0.000 n=10)

               │   old.txt   │              new.txt               │
               │  allocs/op  │ allocs/op   vs base                │
ResourceLogs-8   178.00 ± 0%   52.00 ± 0%  -70.79% (p=0.000 n=10)

@pree-dew
5782-fix-duplicate-logs-across-all-resources: 1. Create scope map wit…
f8f69dc 
…h resource key to map the correct record rules. 2. Add test case with different resource and scope combination
@codecov Codecov
Copy link

codecov bot commented Sep 10, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.5%. Comparing base (42fd8fe) to head (5994633).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5803     +/-   ##
=======================================
- Coverage   84.5%   84.5%   -0.1%     
=======================================
  Files        272     272             
  Lines      22758   22734     -24     
=======================================
- Hits       19250   19226     -24     
  Misses      3165    3165             
  Partials     343     343

see 2 files with indirect coverage changes

@dmathieu dmathieu changed the title 5782-fix-duplicate-logs-across-all-resources: 1. Create scope map wit… Fix duplicate logs across resources Sep 11, 2024
@pree-dew
Copy link
Contributor Author

@dmathieu all checks have passed except this one https://github.com/open-telemetry/opentelemetry-go/actions/runs/10809312925/job/29984151866?pr=5803, it looks flaky as it was passing in previous build.

Let me know your feedback on PR, open to discuss and fix it. :)

@pellared
Copy link
Member

pellared commented Sep 11, 2024 •
edited
Loading

I think this PR removes the performance optimization from #5378. It seems undesirable. Can you add benchmark results to PR description once it is ready for review?

@pellared pellared marked this pull request as draft September 11, 2024 10:00
@pellared pellared marked this pull request as ready for review September 11, 2024 10:01
@pree-dew
Copy link
Contributor Author

@pellared

I think this PR removes the performance optimization from #5378. It seems undesirable. Can you add benchmark results to PR description once it is ready for review?

Sure, I can bring back the pool in the PR if the benchmark doesn't match the performance requirement. Thanks for the feedback.

@pree-dew
Copy link
Contributor Author

pree-dew commented Sep 11, 2024 •
edited
Loading

@pellared Added benchmarks in PR description. Let me know your thoughts on the same.

XSAM
XSAM approved these changes Sep 13, 2024
View reviewed changes
Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

@pree-dew Thanks for the help! Good catch.

It overall looks good to me. I only have the test comment that needs to be solved.

exporters/otlp/otlplog/otlploggrpc/internal/transform/log_test.go Outdated Show resolved Hide resolved
@pree-dew
5782-fix-duplicate-logs-across-all-resources: make first test case be…
27c3cb3 
…tter to have complete assertion match with all attributes
@pree-dew
Merge branch '5782-fix-duplicate-logs-across-all-resources' of github…
918f5f1 
….com:pree-dew/opentelemetry-go into 5782-fix-duplicate-logs-across-all-resources
@pree-dew
5782-fix-duplicate-logs-across-all-resources: resource log order is n…
0cbfdd0 
…ot fixed, so accomodate mapping to correct resource during assertion
@pree-dew
Copy link
Contributor Author

@XSAM @pellared looks like checks of test-race and test-coverage are facing some type of weird issue, it is not reproducible on my github and local, plus it is showing the error at a line that doesn't exists in current version of code. Can you help with this? or am I missing something.

@pellared
Copy link
Member

@pree-dew, can you please sync with main?

@pree-dew
Copy link
Contributor Author

@pellared Sync with main is done

pellared
pellared reviewed Sep 16, 2024
View reviewed changes
Copy link
Member

@pellared pellared left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure how the bug was reproduced and if this a valid scenario.

On the other hand, I am surprised that it looks like this code has better performance (I have not analyzed why).

@pellared
Copy link
Member

Closing per #5782 (comment)

However, it would be good to check why performance has been improved according to the benchmark.

@pellared pellared closed this Sep 16, 2024
@pellared
Copy link
Member

Reopening per #5782 (comment)

@pellared pellared reopened this Sep 17, 2024
XSAM
XSAM reviewed Sep 17, 2024
View reviewed changes
exporters/otlp/otlplog/otlploggrpc/internal/transform/log_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@XSAM XSAM added this to the v1.31.0 milestone Sep 17, 2024
@pellared pellared merged commit 534ce5a into open-telemetry:main Sep 17, 2024
32 checks passed
@dashpole dashpole mentioned this pull request Oct 11, 2024
Merged
dashpole added a commit that referenced this pull request Oct 11, 2024
@dashpole
Release v1.31.0/v0.53.0/v0.7.0/v0.0.10 (#5883)
bc2fe88 
### Added

- Add `go.opentelemetry.io/otel/sdk/metric/exemplar` package which
includes `Exemplar`, `Filter`, `TraceBasedFilter`, `AlwaysOnFilter`,
`HistogramReservoir`, `FixedSizeReservoir`, `Reservoir`, `Value` and
`ValueType` types. These will be used for configuring the exemplar
reservoir for the metrics sdk. (#5747, #5862)
- Add `WithExportBufferSize` option to log batch processor.(#5877)

### Changed

- Enable exemplars by default in `go.opentelemetry.io/otel/sdk/metric`.
Exemplars can be disabled by setting
`OTEL_METRICS_EXEMPLAR_FILTER=always_off` (#5778)
- `Logger.Enabled` in `go.opentelemetry.io/otel/log` now accepts a newly
introduced `EnabledParameters` type instead of `Record`. (#5791)
- `FilterProcessor.Enabled` in
`go.opentelemetry.io/otel/sdk/log/internal/x` now accepts
`EnabledParameters` instead of `Record`. (#5791)
- The `Record` type in `go.opentelemetry.io/otel/log` is no longer
comparable. (#5847)
- Performance improvements for the trace SDK `SetAttributes` method in
`Span`. (#5864)
- Reduce memory allocations for the `Event` and `Link` lists in `Span`.
(#5858)
- Performance improvements for the trace SDK `AddEvent`, `AddLink`,
`RecordError` and `End` methods in `Span`. (#5874)

### Deprecated

- Deprecate all examples under `go.opentelemetry.io/otel/example` as
they are moved to [Contrib
repository](https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/examples).
(#5854)

### Fixed

- The race condition for multiple `FixedSize` exemplar reservoirs
identified in #5814 is resolved. (#5819)
- Fix log records duplication in case of heterogeneous resource
attributes by correctly mapping each log record to it's resource and
scope. (#5803)
- Fix timer channel drain to avoid hanging on Go 1.23. (#5868)
- Fix delegation for global meter providers, and panic when calling
otel.SetMeterProvider. (#5827)
- Change the `reflect.TypeOf` to use a nil pointer to not allocate on
the heap unless necessary. (#5827)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XSAM XSAM XSAM approved these changes

@pellared pellared pellared approved these changes

@dmathieu dmathieu dmathieu approved these changes

@MrAlias MrAlias Awaiting requested review from MrAlias MrAlias is a code owner

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

@hanyuancheung hanyuancheung Awaiting requested review from hanyuancheung

Assignees

@pree-dew pree-dew

Labels
None yet
Projects
Go: Logs (GA)
Status: Done
Milestone
v1.31.0
Development

Successfully merging this pull request may close these issues.

OTLP log export duplicates logs across all present resources
4 participants
@pree-dew @pellared @dmathieu @XSAM