[BEAM-11994] Update ShortIdMap to check preconditions that the payload and startTime are not set, and add unit tests. #14804
Conversation
ajamato
force-pushed
the
bq_metrics_process_wide_short_ids
branch
from
40df4f5 to
e056c82
Compare
Codecov Report
@@ Coverage Diff @@
## master #14804 +/- ##
==========================================
+ Coverage 83.74% 83.79% +0.05%
==========================================
Files 439 439
Lines 59041 59133 +92
==========================================
+ Hits 49442 49551 +109
+ Misses 9599 9582 -17 Continue to review full report at Codecov.
|
ajamato
force-pushed
the
bq_metrics_process_wide_short_ids
branch
from
e056c82 to
aee5e39
Compare
|
Run Java PreCommit |
|
To follow up from the question on the previous PR, why do we need per-metric (vs, say, per-process) start times? And if we don't perhaps we don't need this complexity at all. |
|
I see, thinking about this more. I realize that we will hit bug that OpenCensus had when they tried to share a startTime If you don't report a startTime near and prior to the reporting time of the first timeseries.create call, you can get weird issues in the way that stackdriver does precomputes, interpolation, etc. I actually had to fix this in their github in the past I think the best thing to do is to set the startTime for each MonitoringInfo, in the process_wide MetricContainer only. Then it can be added and stored in the ShortIdMap. This would require the process_wide MetricContainer to somehow know it's the first time it's encountered the MonitoringInfo. So I will make the process_wide MetricContainer reference the ShortIdMap and check if the MonitoringInfo exists first before generating the startTime. What do you think of this approach? |
|
Thanks for clarifying the motivation. I don't think it makes sense to add complexity to Beam metrics to work around a bug in OpenCensus. Instead, whatever code is reporting to OpenCensus should have the hack to assign a reasonable start time value the first time it reports a metric. |
|
What is the next step on this PR? Should we close it based on the comment thread? |
|
Not waiting for review, discussed offline. Need to make some changes. Will get to it in in about 3 weeks. |
ajamato
force-pushed
the
bq_metrics_process_wide_short_ids
branch
2 times, most recently
from
f86b245 to
6553be4
Compare
…s in the MonitoringInfo (urn and labels).
ajamato
force-pushed
the
bq_metrics_process_wide_short_ids
branch
from
6553be4 to
7cfd330
Compare
|
Failing test doesn't looks like it is due to this PR |
|
https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/18041/#showFailuresLink org.apache.beam.runners.flink.FlinkSavepointTest.testSavepointRestoreLegacy |
ajamato
changed the title
|
Run Java PreCommit |
|
R: @robertwb I revised the PR, Let's pick up and complete this review again. |
|
Run Java PreCommit |
|
Run Java PreCommit |
6 similar comments
|
Run Java PreCommit |
|
Run Java PreCommit |
|
Run Java PreCommit |
|
Run Java PreCommit |
|
Run Java PreCommit |
|
Run Java PreCommit |
[BEAM-11994] Update ShortIdMap to check preconditions that the payload and startTime are not set, and add unit tests.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
ValidatesRunnercompliance status (on master branch)Examples testing status on various runners
Post-Commit SDK/Transform Integration Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.