[ETW] Fix tracer initialization order #3824

beep-boopp · 2026-01-21T18:41:35Z

Description

The ETWTracer was incorrectly starting in a closed state due to member initialization order.

isClosed_ was declared after provHandle, so its default initializer ({true}) was running after provHandle's constructor had already set it to false. This overwrote the correct state.

I moved isClosed_ before provHandle to ensure it initializes first.

Testing

Added ConstructorInitializesToOpenState to etw_tracer_test.cc.
I added a friend declaration in the header to verify that isClosed_ is correctly false immediately after the tracer is instantiated.

Type of Change

Bug fix (non-breaking change which fixes an issue)

Checklist

My code follows the style guidelines of this project
I have added tests that prove my fix is effective

linux-foundation-easycla · 2026-01-21T18:41:44Z

The committers listed above are authorized under a signed CLA.

✅ login: beep-boopp / name: V Prajwal (0d18db0, 596b6af)
✅ login: marcalff / name: Marc Alff (8966698)

lalitb · 2026-01-21T18:55:36Z

.vscode/settings.json

@@ -0,0 +1,3 @@
+{
+    "cmake.sourceDirectory": "/wsl.localhost/Ubuntu/home/percy/opentelemetry-cpp"
+}


seems to be incorrectly checked in?

lalitb · 2026-01-21T18:56:30Z

exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h

   * @brief Provider Handle
   */
-  ETWProvider::Handle &provHandle;
+  //change made in pr 


nit -this line seems unnecessary.

lalitb · 2026-01-21T18:57:32Z

exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h


-  std::atomic<bool> isClosed_{true};
+  // Fix: Declare isClosed_ before provHandle so it initializes to true 
+  // *before* initProvHandle() runs.


please remove all the above comments - we don't need PR change history as part of comment.

ThomsonTan · 2026-01-21T19:06:45Z

exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h

-  std::atomic<bool> isClosed_{true};
-
  /**
+   * @brief ETWProvider is a singleton that aggregates all ETW writes.


Is this line duplicated?

ThomsonTan · 2026-01-21T19:09:27Z

exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h


  /**
   * @brief Provider Handle
   */


Move the above 3 lines comment with provHandle?

ThomsonTan · 2026-01-21T19:15:58Z

@beep-boopp thanks for the fix. Please also address the CI issue and comments, looks good otherwise.

beep-boopp · 2026-01-21T19:46:57Z

@beep-boopp thanks for the fix. Please also address the CI issue and comments, looks good otherwise.

I pushed a new commit, waiting for approval. The CI shows 'Awaiting Approval' since this is my first contribution, but the build should be green once approved. Ready to merge. Once again thanks for all the help

ThomsonTan · 2026-01-22T03:19:54Z

@beep-boopp thanks for the fix. Please also address the CI issue and comments, looks good otherwise.

I pushed a new commit, waiting for approval. The CI shows 'Awaiting Approval' since this is my first contribution, but the build should be green once approved. Ready to merge. Once again thanks for all the help

I approved the CI. Seems more code format needed.

beep-boopp · 2026-01-22T03:30:34Z

@beep-boopp thanks for the fix. Please also address the CI issue and comments, looks good otherwise.

I pushed a new commit, waiting for approval. The CI shows 'Awaiting Approval' since this is my first contribution, but the build should be green once approved. Ready to merge. Once again thanks for all the help

I approved the CI. Seems more code format needed.

Thanks for the approval! I actually just force-pushed a clearer fix (switched to a getter instead of friend class and squashed the old commits). The formatting should be solid now too, but let me know if the CI catches anything else .

codecov · 2026-01-22T03:48:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.96%. Comparing base (356fc9e) to head (8966698).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3824   +/-   ##
=======================================
  Coverage   89.96%   89.96%           
=======================================
  Files         225      225           
  Lines        7170     7170           
=======================================
  Hits         6450     6450           
  Misses        720      720

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

marcalff

Thanks for the patch.

The test looks good, but I disagree with the fix in etw_tracer.h

A better solution is to remove method initProvHandle() entirely: calling a method from the constructor was a bad idea in the first place.

beep-boopp · 2026-01-22T12:58:28Z

Thanks for the patch.

The test looks good, but I disagree with the fix in etw_tracer.h

A better solution is to remove method initProvHandle() entirely: calling a method from the constructor was a bad idea in the first place.

Thanks for the clarification , I see where you’re coming from, and I agree that calling a helper like initProvHandle() from the constructor isn’t ideal.
I’ll refactor this to remove initProvHandle() entirely and move the initialization directly into the constructor, establishing the invariants in one place.
I’ll also ensure the update complies with the project’s clang-format rules so the CI / Format check passes and incorporate both changes together in the next commit

ThomsonTan · 2026-01-22T15:54:46Z

Thanks for the patch.

The test looks good, but I disagree with the fix in etw_tracer.h

A better solution is to remove method initProvHandle() entirely: calling a method from the constructor was a bad idea in the first place.

Replace the call initProvHandle() by some other function call like the inner function etwProvider().open() should be the same effect? I suggest the we fix the the issue with the minimum change and do the refactor later as we revisit that in broader scope on similar issues.

lalitb · 2026-01-22T16:08:11Z

I suggest the we fix the the issue with the minimum change and do the refactor later as we revisit that in broader scope on similar issues.

+1

beep-boopp · 2026-01-22T16:40:55Z

Thanks for the patch.
The test looks good, but I disagree with the fix in etw_tracer.h
A better solution is to remove method initProvHandle() entirely: calling a method from the constructor was a bad idea in the first place.

Replace the call initProvHandle() by some other function call like the inner function etwProvider().open() should be the same effect? I suggest the we fix the the issue with the minimum change and do the refactor later as we revisit that in broader scope on similar issues.

I believe my latest commit already does exactly this: I removed initProvHandle() entirely and replaced the call in the constructor with a direct call to etwProvider().open(provId, encoding), keeping the change minimal.
Please let me know if the current implementation matches what you had in mind.

beep-boopp · 2026-01-23T11:12:50Z

@lalitb Thanks for the help with the updates.
Just to confirm the current state: I have implemented the requested change by removing initProvHandle() entirely and inlining the logic into the constructor to keep the fix minimal.
The latest build failure appears to be an infrastructure issue (HTTP 500 / Timeout during Windows dependency installation) rather than a code error. Could you please re-trigger the CI?
If you spot any other adjustments needed, just let me know.

marcalff · 2026-01-23T15:14:12Z

exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h

-    return etwProvider().open(provId, encoding);
-  }
-
-public:


Removed the public: declaration for the constructor ?

This breaks the build, see CI.

marcalff · 2026-01-23T15:15:17Z

exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h


+  // Order matters: isClosed_ must initialize to true BEFORE provHandle
+  // runs its initialization logic.
+  std::atomic<bool> isClosed_{true};


There is no need to more the declaration around for isClosed_, please revert this change back to the original code.

marcalff

See build breaks in CI.

beep-boopp · 2026-01-23T17:13:53Z

See build breaks in CI.

@marcalff Thanks for the detailed review! Good catch on the missing public: modifier, I definitely missed that while cleaning up the helper function.
I’ve restored the access modifier and reverted isClosed_ to its original position as requested. It should be good to go now.

beep-boopp · 2026-01-23T18:25:33Z

Apologies for the extra CI noise on this PR and new commit. I noticed the Windows Bazel build failed due to an MSVC-specific namespace resolution error (C2653) in the new test case. I've updated it to use the fully qualified namespace opentelemetry::exporter::etw:: to ensure compatibility across all builders. Ready for another look whenever you have the time

marcalff

Thanks for the fix.

[ETW] Fix tracer initialization order (open-telemetry#3824)

beep-boopp requested a review from a team as a code owner January 21, 2026 18:41

lalitb reviewed Jan 21, 2026

View reviewed changes

beep-boopp force-pushed the fix/etw-initialization-order branch 2 times, most recently from 927910d to f6b3576 Compare January 21, 2026 19:05

ThomsonTan reviewed Jan 21, 2026

View reviewed changes

lalitb approved these changes Jan 21, 2026

View reviewed changes

ThomsonTan reviewed Jan 21, 2026

View reviewed changes

beep-boopp force-pushed the fix/etw-initialization-order branch 2 times, most recently from 6ab7c2f to 5302f1f Compare January 21, 2026 19:25

beep-boopp force-pushed the fix/etw-initialization-order branch from 5302f1f to 109d27e Compare January 22, 2026 03:25

marcalff requested changes Jan 22, 2026

View reviewed changes

Refactor: Inline initProvHandle and fix formatting

18503ab

beep-boopp force-pushed the fix/etw-initialization-order branch from 7f57e9d to 18503ab Compare January 22, 2026 14:02

marcalff reviewed Jan 23, 2026

View reviewed changes

marcalff requested changes Jan 23, 2026

View reviewed changes

fix: restore public access and revert isClosed_ order

0d18db0

beep-boopp force-pushed the fix/etw-initialization-order branch from b16c146 to 0d18db0 Compare January 23, 2026 17:06

test: fix MSVC namespace resolution and add constructor test

596b6af

beep-boopp requested a review from marcalff January 24, 2026 09:42

Merge branch 'main' into fix/etw-initialization-order

8966698

marcalff approved these changes Jan 24, 2026

View reviewed changes

marcalff changed the title ~~Fix/etw initialization order~~ [ETW] Fix tracer initialization order Jan 24, 2026

marcalff merged commit 7fb5a00 into open-telemetry:main Jan 24, 2026
113 of 114 checks passed

malkia added a commit to malkia/opentelemetry-cpp that referenced this pull request Jan 24, 2026

Merge pull request #638 from open-telemetry/main

c3aee74

[ETW] Fix tracer initialization order (open-telemetry#3824)

@@ @@ -0,0 +1,3 @@ @@
+              {
+                  "cmake.sourceDirectory": "/wsl.localhost/Ubuntu/home/percy/opentelemetry-cpp"
+              }

                
                    No newline at end of file

[ETW] Fix tracer initialization order #3824

[ETW] Fix tracer initialization order #3824

Conversation

beep-boopp commented Jan 21, 2026

Description

Testing

Type of Change

Checklist

Uh oh!

linux-foundation-easycla bot commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lalitb Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

lalitb Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

ThomsonTan Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

ThomsonTan Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

ThomsonTan commented Jan 21, 2026

Uh oh!

beep-boopp commented Jan 21, 2026

Uh oh!

ThomsonTan commented Jan 22, 2026

Uh oh!

beep-boopp commented Jan 22, 2026

Uh oh!

codecov bot commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

beep-boopp commented Jan 22, 2026

Uh oh!

ThomsonTan commented Jan 22, 2026

Uh oh!

lalitb commented Jan 22, 2026

Uh oh!

beep-boopp commented Jan 22, 2026

Uh oh!

beep-boopp commented Jan 23, 2026

Uh oh!

marcalff Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

marcalff Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

beep-boopp commented Jan 23, 2026

Uh oh!

beep-boopp commented Jan 23, 2026

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linux-foundation-easycla bot commented Jan 21, 2026 •

edited

Loading

lalitb Jan 21, 2026 •

edited

Loading

codecov bot commented Jan 22, 2026 •

edited

Loading