[Tracer] (Event Grid 3/5) Add outbound end-to-end coverage - #8913
pablomartinezbernardo wants to merge 1 commit into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BenchmarksBenchmark execution time: 2026-10-02 13:31:09 Comparing candidate commit e74ac08 in PR branch Found 0 performance improvements and 12 performance regressions! Performance is the same for 60 metrics, 0 unstable metrics, 75 known flaky benchmarks, 51 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8913) and master. ✅ No regressions detected |
6962620 to
2674581
Compare
49fa28b to
deb530a
Compare
2674581 to
ab8152d
Compare
deb530a to
2dcd501
Compare
ab8152d to
2f6ba44
Compare
2dcd501 to
2adf2d3
Compare
2f6ba44 to
9818224
Compare
2adf2d3 to
5e098c3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e098c35ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
5e098c3 to
7f77047
Compare
9818224 to
7a0f908
Compare
3c34a51 to
73fd3c4
Compare
73fd3c4 to
7a79f7a
Compare
7a0f908 to
7af02af
Compare
7a79f7a to
183a4ea
Compare
There was a problem hiding this comment.
should we add those to the macOS slnf as well ? if there is no major incompatibility, we should.
There was a problem hiding this comment.
Correct, added!
|
|
||
| namespace Samples.AzureEventGrid | ||
| { | ||
| public enum TestMode |
There was a problem hiding this comment.
what's usually done in other tests is that the sample app runs once and does all the possible operations (see sql tests for instance, they send a bunch of requests with all instrumented methods). Ten we have one single snapshot that contains all spans.
IDK if your way is better or worse. The upside of having everything at once is that the setup cost is only paid once (tests probably run faster, which is a significant factor given the current situation), the sample code is simpler, and there is no hidden param.
The upside I see to your approach is that assertions can be more precise and if something breaks, the error message is clearer.
There was a problem hiding this comment.
Regarding the hidden param: While it is true that the test needs to understand how this parameter works, tests always need to know how the sample app works. As for simplicity, I think having switch cases makes things easier to understand even if more verbose.
As for startup time, it is true that fewer processes would cause less overhead, but I'm not sure how relevant that is in the overall time. Let's leave this open for others to chime in!
There was a problem hiding this comment.
My inclination is towards @vandonr. Startup/shutdown costs is a big part of the runtime in general, and also one of the main sources of flakiness. I would personally be strongly inclined to follow the same standard pattern, and use a snapshot. The main thing to get right with that is that you have a Stable order of spans, based on resources names etc (you can customize that). You can also wrap each event Type with a concrete-named manual span if working out "which span is which" is the issue.
The main time we would choose to have a "test mode" is to reduce the number of samples we create. e.g. we have a Samples.Console which is explicitly running many different types of test, to reduce the build times associated with building samples, but it seems like that's very different to this scenario
There was a problem hiding this comment.
Sounds good, done!
183a4ea to
6443b33
Compare
7af02af to
05dc004
Compare
6443b33 to
4624d38
Compare
4624d38 to
68761f7
Compare
3a30bb0 to
c2d2c85
Compare
471832a to
920c7e1
Compare
c2d2c85 to
52645d7
Compare
920c7e1 to
2ad6d60
Compare
2ad6d60 to
84a8fbd
Compare
52645d7 to
525e44f
Compare
84a8fbd to
e74ac08
Compare

Summary of changes
Adds end-to-end coverage for outbound Azure Event Grid instrumentation across
Azure.Messaging.EventGridandAzure.Messaging.EventGrid.Namespaces.Reason for change
Verify that the outbound instrumentation introduced earlier in this Graphite stack produces the expected spans across supported SDK versions and APIs.
Implementation details
Test coverage
Other details
This PR is part of a larger Graphite stack.