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

Add isolate module #5861

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Add isolate module #5861

wants to merge 11 commits into from

Conversation

pellared
Copy link
Member

@pellared pellared commented Jul 5, 2024

Blocked by:

Fixes #5823

Benchmark results:

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/contrib/processors/isolate
cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
BenchmarkLogProcessor/Record_without_shared_data-16         	17212789	        75.12 ns/op	       0 B/op	       0 allocs/op
BenchmarkLogProcessor/Record_with_shared_data-16            	 4411912	       263.3 ns/op	     416 B/op	       2 allocs/op

@pellared pellared self-assigned this Jul 5, 2024
Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 6 lines in your changes missing coverage. Please review.

Project coverage is 64.6%. Comparing base (a952931) to head (648e4fa).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5861     +/-   ##
=======================================
- Coverage   64.6%   64.6%   -0.1%     
=======================================
  Files        201     202      +1     
  Lines      12587   12601     +14     
=======================================
+ Hits        8134    8142      +8     
- Misses      4216    4221      +5     
- Partials     237     238      +1     
Files Coverage Δ
processors/isolate/processor.go 57.1% <57.1%> (ø)

return p.Processor.Enabled(ctx, record)
}

var defaultProcessor = noopProcessor{}
Copy link
Member Author

@pellared pellared Jul 5, 2024

Choose a reason for hiding this comment

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

Here is another usage of an no-op processor in a processor implementation.
I think we can think about adding it the the SDK.

See also: #5817 (comment)

EDIT: I created open-telemetry/opentelemetry-go#5580

@pellared pellared marked this pull request as ready for review July 8, 2024 09:00
@pellared pellared requested a review from a team July 8, 2024 09:00
@pellared pellared changed the title [WIP] Add isolating log processor Add isolating log processor Jul 8, 2024
@pellared pellared changed the title Add isolating log processor Add isolate module Jul 8, 2024
@pellared pellared added this to the v1.29.0 milestone Jul 8, 2024
dmathieu

This comment was marked as resolved.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

It's not clear to me this is needed currently. We still do not comply with specification regarding our log record type begin passed to the log processor: open-telemetry/opentelemetry-go#5219. The resolution of that may make this type of log processor redundant and unneeded.

I would prefer we resolve prior to adding this kind of extension.

@pellared
Copy link
Member Author

pellared commented Jul 8, 2024

It's not clear to me this is needed currently. We still do not comply with specification regarding our log record type begin passed to the log processor: open-telemetry/opentelemetry-go#5219. The resolution of that may make this type of log processor redundant and unneeded.

I would prefer we resolve prior to adding this kind of extension.

Even if the processor would to accept a fully mutable record - this processor would still be helpful. However, we can postpone this if this is your preference, then please move it to "blocked".

@MrAlias
Copy link
Contributor

MrAlias commented Jul 8, 2024

Even if the processor would to accept a fully mutable record - this processor would still be helpful. However, we can postpone this if this is your preference, then please move it to "blocked".

With our changes and this still pending, I would prefer to not add this as it has a good potential to be cruft.

@pellared pellared added the blocked: specification Waiting on clarification of the OpenTelemetry specification before progress can be made label Jul 8, 2024
@pellared pellared removed this from the v1.29.0 milestone Jul 8, 2024
@pellared pellared marked this pull request as draft July 8, 2024 15:22
@dmathieu dmathieu dismissed their stale review July 9, 2024 07:35

moved back to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked: specification Waiting on clarification of the OpenTelemetry specification before progress can be made
Projects
Status: Blocked
Development

Successfully merging this pull request may close these issues.

Add isolating log record processor
3 participants