TRANSCEIVER-13.1 Add distinct performance monitoring interval#5294
TRANSCEIVER-13.1 Add distinct performance monitoring interval#5294jnelson-sj wants to merge 2 commits intoopenconfig:mainfrom
Conversation
avoid overwriting samplingInterval
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability of performance monitoring tests by isolating the sampling interval configuration. By creating a distinct variable for performance monitoring, the code avoids side effects that could impact other parts of the test suite that rely on the global sampling interval. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the TestLowPowerMode function to utilize a specific sampling interval, perfMonSamplingInterval, when creating sample streams for optical channel output power. The interval is retrieved from the device state if the corresponding deviation is not enabled. A critical bug was identified in the implementation where the integer value returned from gNMI was cast directly to time.Duration, resulting in nanosecond-scale intervals instead of seconds. A code suggestion was provided to correctly scale the duration by time.Second.
| perfMonSamplingInterval := samplingInterval | ||
| if !deviations.SkipOpticalChannelOutputPowerInterval(dut) { | ||
| samplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) | ||
| perfMonSamplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) |
There was a problem hiding this comment.
The value returned by gnmi.Get for the interval leaf is an integer (typically representing seconds or milliseconds depending on the device implementation). Casting it directly to time.Duration treats the value as nanoseconds, which results in an extremely short sampling interval (e.g., 30ns instead of 30s). This will likely cause samplestream.Next() to return stale data or nil because it won't wait long enough for a new sample to be published by the DUT. Please multiply the value by the appropriate time unit, such as time.Second.
| perfMonSamplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) | |
| perfMonSamplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) * time.Second |
There was a problem hiding this comment.
The type of the interval parameter is a stat-interval, which is defined as
typedef stat-interval {
type uint64;
units nanoseconds;
description
"A time interval over which a set of statistics is computed.
A common usage is to report the interval over which
avg/min/max stats are computed and reported.";
}
Two interesting caveats, however:
- The zrp version of this test does multiply the interval by time.Seconds
- The Arista OpenConfig implementation currently reports duration in seconds, not nano-seconds
I think the correct thing to do is to leave this as is (assumes nanoseconds), to remove the scaling in the zrp test, and to change the Arista implementation to report nanoseconds.
2 participants
Use a distinct performance monitoring interval to avoid overwriting the global samplingInterval variable.