feat: stabilization of opentelemetry-etw-logs #247

psandana · 2025-04-11T21:03:26Z

Fixes #
Design discussion issue (if applicable) #

Changes

Changing from a trait extension to a LogProcessor factory.

Merge requirement checklist

CONTRIBUTING guidelines followed
Unit tests added/updated (if applicable)
Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
Changes in public API reviewed (if applicable)

codecov · 2025-04-11T21:31:52Z

Codecov Report

Attention: Patch coverage is 99.05063% with 3 lines in your changes missing coverage. Please review.

Project coverage is 49.5%. Comparing base (ea3320a) to head (ebf17c1).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...pentelemetry-etw-logs/src/logs/exporter/options.rs	97.2%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #247     +/-   ##
=======================================
+ Coverage   48.0%   49.5%   +1.5%     
=======================================
  Files         60      60             
  Lines       8576    8760    +184     
=======================================
+ Hits        4121    4342    +221     
+ Misses      4455    4418     -37

☔ 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.

opentelemetry-etw-logs/src/logs/exporter_options.rs

psandana · 2025-06-02T17:11:20Z

Thanks! The direction looks good, here are few things we need to reconsider

Please remove all changes unrelated to changing from "with_etw" to the processor builder pattern. Large PRs gets harder to review, especially when new commits are pushed.

Unsure if we should use a totally different approach of returning Errors here - I suggest to stick with returning Box for now.

user-events did recent refactor to support the new processor builder, see if we can make it consistent with that. I think we should simplify code bit more to make it easier to follow, left some comments about this in couple of places.

Hi @cijothomas . I reworked the PR using the feedback provided. Errors are Boxed and simplified things. Please review.

opentelemetry-etw-logs/Cargo.toml

cijothomas · 2025-06-04T18:50:33Z

opentelemetry-etw-logs/Cargo.toml

@@ -11,15 +11,19 @@ keywords = ["opentelemetry", "log", "trace", "etw"]
 license = "Apache-2.0"

 [dependencies]
-tracelogging_dynamic = "1.2.4"
+chrono = { version = "0.4.40", default-features = false, features = [


its hard to tell if this file actually has changes or not..looks like just re-arranging ? please remove unrelated changes from this PR.

I won't trim this PR further. Please, accept this PR as is. Subsequent ones should be small, incremental changes.

Sorry I don't follow. Does this file actually have any changes or just re-arranging the order?
Its putting unnecessary burden on reviewers to closely check and confirm if this is actually changed or not.

Well, this is a limitation of the diff tool and how git works. At this time, is only reordering.

The project does not have a toml formatting, causing this kind of inconsistencies.

Its putting unnecessary burden on reviewers to closely check and confirm if this is actually changed or not.

I’m with @cijothomas on this. Simplifying the PR for review not only helps reviewers but also benefits the author by increasing the chances of a quicker merge. In this case, the only intended change appears to be the removal of microbench from the dev dependencies. Ideally, that should have been the sole modification, without reordering the other dependencies.

The project does not have a toml formatting, causing this kind of inconsistencies.

It’s usually a good idea to stick to the existing style, formatting, or conventions used in the project when sending in PRs. If there doesn’t seem to be a clear standard yet, it might be better to open a separate PR to introduce one, rather than mixing style changes into an unrelated change.

I know it may help to have smaller targeted PRs, but at the same time, there has been a lot of back and forth in design and decisions that makes it difficult to track in smaller PRs at this moment.

We added more deps on this PR initially, then we removed some of them. That, in combination with leaving the files better than before, led to the changes as you see today. For me, this is a motto: leave everything better than was before. I know this can be noisier to review, but the idea is to pay this cost once (now), than in the future.

Totally fine with the idea of improving everything, but please do it only in own, separate PRs.

cijothomas · 2025-06-04T21:58:19Z

opentelemetry-etw-logs/src/logs/processor.rs

+    ///
+    /// The resulting name must be a valid CommonSchema 4.0 TraceLoggingDynamic event name. Otherwise,
+    /// the default "Log" ETW event name will be used.
+    pub fn etw_event_name_from_callback(


Please see the discussion here : #275 (comment)

We need to decide how best to handle this.

I saw that discussion. I propose we merge this PR without releasing a version yet. Once we agree upon a design, we change in both etw and ue exporters.

please add a TODO or something to indicate this needs to be discussed further. Okay to merge with this.

opentelemetry-etw-logs/src/logs/processor.rs

cijothomas · 2025-06-05T16:05:49Z

opentelemetry-etw-logs/src/logs/processor.rs

+        use tracing_subscriber::prelude::*;
+
+        let processor = Processor::builder("provider-name")
+            .etw_event_name_from_callback(|_| "CustomEvent")


unsure what is the validation this test is doing? I see "CustomEvent" is returned, but there is no validation.. Is this test just to ensure no panic etc. occurs? Please add a small comment so its easy to tell the intent.

It is an integration test at this moment. Renamed to tracing_integration_test. We can move it to the tests folder (we should create it) if you prefer, either on this PR or a new one.

My comment was more about - "There are not asserts in the test, so what is the purpose of the test"?

cijothomas · 2025-06-05T16:10:04Z

opentelemetry-etw-logs/src/logs/exporter/options.rs

+            .etw_event_name_from_callback(|log_record| log_record.event_name().unwrap_or_default());
+
+        let result = options.get_etw_event_name(&log_record);
+        assert_eq!(result, "Log");


interesting that "Log" is still the name, though the callback is always returning something... Is this trying to test that invalid_event_name_from_callback_will_use_default ?

It is not surprising, the name initially is empty, thus the validation, in its current basic form, fails and default to "Log".

Got it. It's better to have a targeted test for the same. invalid_event_name_from_callback_will_use_default would be my suggestion.

Not a blocker.

cijothomas · 2025-06-05T16:12:36Z

opentelemetry-etw-logs/src/logs/exporter/options.rs

+        let result = options.get_etw_event_name(&log_record);
+        assert_eq!(result, "Log");
+
+        log_record.set_target("target-name");


what happens if we pass empty string to target?

Depends on the callback given. Assuming the callback is returning the target, the current validation will discard it because it is empty and return the default "Log".

opentelemetry-etw-logs/src/logs/exporter/part_c.rs

opentelemetry-etw-logs/README.md

cijothomas · 2025-06-12T01:03:00Z

opentelemetry-etw-logs/src/logs/exporter/options.rs

+    }
+
+    #[test]
+    fn test_get_event_name_from_callback_returning_target() {


not sure what are these tests trying to test. For the callback, we just take the name from the callback - does it matter where/how the callback is returning the value from? It can do it from event_name or target or static.. doesn't matter.

cijothomas

LGTM

feat: Use "Log" as default event name

488fc72

psandana requested a review from a team as a code owner April 11, 2025 21:03

psandana added 2 commits April 11, 2025 17:23

make clippy happy

fe0926c

fix tests

f84eba6

psandana added 18 commits April 11, 2025 18:05

Merge branch 'main' into log-by-default

b196973

Introducing ExporterOptions to ETW log exporter

f9d3d13

trying frozen_collections and String instead of &str

0854965

build fix

50b61c0

fmt

b47781a

improving mapping

cdeb6ba

refactor get_event_name

d31264b

rename to get_etw_event_name

b826bd9

fmt

e219cfb

implement missing logic

a4cc027

using generics

bbaa7b4

remove frozen_collections

0ced617

remove duplication

e8c10f3

fmt

b1e4175

fix typo

86e829f

small perf optimization: return &str

efb3424

use � for key

3b26295

More Cow in provider and default event names

968c1f8

psandana changed the title ~~feat: Use "Log" as default event name~~ feat: Add "Log" as default event name Apr 22, 2025

psandana changed the title ~~feat: Add "Log" as default event name~~ feat: Event mapping support on target attribute Apr 22, 2025

martintmk reviewed Apr 23, 2025

View reviewed changes

opentelemetry-etw-logs/src/logs/exporter_options.rs Outdated Show resolved Hide resolved

psandana added 3 commits April 23, 2025 12:26

small reword

59f7660

Cow ➡ Into<Cow>

e4b85a4

fmt

6db9260

psandana commented Apr 25, 2025

View reviewed changes

opentelemetry-etw-logs/src/logs/exporter_options.rs Outdated Show resolved Hide resolved

psandana added 5 commits May 30, 2025 13:04

remove wrong user events change

12cb308

fix ue changelog

4cc6955

improve CHANGELOG

d54d24e

remove unused dep

61fa832

migrate validation from Options to ProcessorBuilder

dd3ed29

cijothomas reviewed Jun 4, 2025

View reviewed changes

opentelemetry-etw-logs/Cargo.toml Outdated Show resolved Hide resolved

cijothomas reviewed Jun 4, 2025

View reviewed changes

remove unused dep

62b709c

cijothomas reviewed Jun 4, 2025

View reviewed changes

cijothomas reviewed Jun 5, 2025

View reviewed changes

opentelemetry-etw-logs/src/logs/processor.rs Outdated Show resolved Hide resolved

cijothomas reviewed Jun 5, 2025

View reviewed changes

opentelemetry-etw-logs/src/logs/processor.rs Outdated Show resolved Hide resolved

cijothomas reviewed Jun 5, 2025

View reviewed changes

opentelemetry-etw-logs/src/logs/processor.rs Outdated Show resolved Hide resolved

cijothomas reviewed Jun 5, 2025

View reviewed changes

opentelemetry-etw-logs/src/logs/exporter/part_c.rs Outdated Show resolved Hide resolved

psandana added 2 commits June 5, 2025 12:45

renaming variable

230704c

renaming tracing_test to tracing_integration_test

87becde

utpilla reviewed Jun 5, 2025

View reviewed changes

opentelemetry-etw-logs/README.md Outdated Show resolved Hide resolved

psandana and others added 7 commits June 5, 2025 13:54

removing hyphen from provider_name

ee8d649

format

dc48528

empty commit: triggering pipeline

47edbee

Merge branch 'main' into log-by-default

9f46eea

Merge branch 'main' into log-by-default

912240a

remove unintended project

3970bcb

Merge branch 'main' into log-by-default

ebf17c1

cijothomas reviewed Jun 12, 2025

View reviewed changes

cijothomas approved these changes Jun 12, 2025

View reviewed changes

cijothomas merged commit d8f105e into open-telemetry:main Jun 16, 2025
22 checks passed

feat: stabilization of opentelemetry-etw-logs #247

feat: stabilization of opentelemetry-etw-logs #247

Uh oh!

Conversation

psandana commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Merge requirement checklist

Uh oh!

codecov bot commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

psandana commented Jun 2, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cijothomas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

psandana commented Apr 11, 2025 •

edited

Loading

codecov bot commented Apr 11, 2025 •

edited

Loading