Skip to content

dym: make metrics ID inner field public for test/debug#44115

Merged
mathetake merged 1 commit into
envoyproxy:mainfrom
mathetake:metrics
Mar 26, 2026
Merged

dym: make metrics ID inner field public for test/debug#44115
mathetake merged 1 commit into
envoyproxy:mainfrom
mathetake:metrics

Conversation

@mathetake
Copy link
Copy Markdown
Member

Commit Message: dym: make metrics ID inner field publish for test/debug
Additional Description:

Otherwise, we cannot mock the logic with the metrics ID without using transmute unsage stuff

Risk Level: n/a
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com>
@mathetake mathetake marked this pull request as ready for review March 26, 2026 00:29
@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #44115 was opened by mathetake.

see: more, trace.

@mathetake mathetake enabled auto-merge (squash) March 26, 2026 00:35
@mathetake mathetake changed the title dym: make metrics ID inner field publish for test/debug dym: make metrics ID inner field public for test/debug Mar 26, 2026
@mathetake mathetake disabled auto-merge March 26, 2026 00:35
@mathetake mathetake enabled auto-merge (squash) March 26, 2026 00:36
@mathetake mathetake merged commit f4ac472 into envoyproxy:main Mar 26, 2026
28 checks passed
@mathetake mathetake deleted the metrics branch March 26, 2026 00:57
@akonradi
Copy link
Copy Markdown
Contributor

Before this change the metric ID values could usefully be used as witness types, so if a EnvoyCounterId existed, the caller could be guaranteed it was valid. Now anybody can construct them. That's useful for testing but makes it easier to write incorrect code.

A pattern I've seen before for this is to have a supertrait that provide associated types for IDs like this. The production type that implements the trait and supertrait would use its real ID types and the test type that implements the trait would provide its own test-only ID types that are easier to construct.

@mathetake
Copy link
Copy Markdown
Member Author

@akonradi how about providing a constructor only available in #[cfg(test)]?

@akonradi
Copy link
Copy Markdown
Contributor

That won't work either, unfortunately. #[cfg(test)] only gets set on the crate under test, not on any of its dependencies. The way to do this is to add a Cargo "feature" that enables additional code. That's a common pattern; tokio, for example, has a test-util feature that allows you to advance its mocked clock. We could do something similar here.

TAOXUY pushed a commit to TAOXUY/envoy that referenced this pull request Apr 1, 2026
)

Commit Message:  dym: make metrics ID inner field publish for test/debug
Additional Description:

Otherwise, we cannot mock the logic with the metrics ID without using
transmute unsage stuff

Risk Level: n/a
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com>
Signed-off-by: Xuyang Tao <taoxuy@google.com>
citrus7 pushed a commit to citrus7/envoy that referenced this pull request Apr 1, 2026
)

Commit Message:  dym: make metrics ID inner field publish for test/debug
Additional Description:

Otherwise, we cannot mock the logic with the metrics ID without using
transmute unsage stuff

Risk Level: n/a
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com>
Signed-off-by: Jonathan Wu <jtwu@google.com>
nshipilov pushed a commit to nshipilov/envoy that referenced this pull request Apr 13, 2026
)

Commit Message:  dym: make metrics ID inner field publish for test/debug
Additional Description:

Otherwise, we cannot mock the logic with the metrics ID without using
transmute unsage stuff

Risk Level: n/a
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com>
Signed-off-by: Nick Shipilov <nick.shipilov.n@gmail.com>
krinkinmu pushed a commit to grnmeira/envoy that referenced this pull request Apr 20, 2026
)

Commit Message:  dym: make metrics ID inner field publish for test/debug
Additional Description:

Otherwise, we cannot mock the logic with the metrics ID without using
transmute unsage stuff

Risk Level: n/a
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Takeshi Yoneda <tyoneda@netflix.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants