You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/10/06 01:18:54 UTC
[GitHub] [beam] ihji opened a new pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
ihji opened a new pull request #13017:
URL: https://github.com/apache/beam/pull/13017
…ncy logging
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/29787b38b594e29428adaf230b45f9b33e24fa66?el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `72.72%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.45% -0.04%
==========================================
Files 454 455 +1
Lines 54813 55028 +215
==========================================
+ Hits 45215 45375 +160
- Misses 9598 9653 +55
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `81.02% <44.44%> (-13.27%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <66.66%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [sdks/python/apache\_beam/utils/retry.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvcmV0cnkucHk=) | `86.11% <0.00%> (-0.93%)` | :arrow_down: |
| [...ks/python/apache\_beam/runners/worker/statecache.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvc3RhdGVjYWNoZS5weQ==) | `96.18% <0.00%> (-0.54%)` | :arrow_down: |
| ... and [13 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...764196c](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/6f58212d42ce387a14b6d7738977cf1380a655aa?el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `83.73%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.48% -0.01%
==========================================
Files 454 455 +1
Lines 54797 55028 +231
==========================================
+ Hits 45202 45389 +187
- Misses 9595 9639 +44
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <75.00%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `93.43% <91.66%> (-0.86%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `82.70% <100.00%> (+2.86%)` | :arrow_up: |
| [sdks/python/apache\_beam/testing/test\_utils.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdGVzdGluZy90ZXN0X3V0aWxzLnB5) | `100.00% <100.00%> (ø)` | |
| ... and [19 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...6bc49b1](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r501954641
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
We do plan on building a few system metrics for GCP IO API request call latency which use histograms. Though, it doesn't need to use the user metric framework itself, its a reasonable way to implement it to use a lot of the same techniques.
I asked @ihji to use the metric framework for the logger, as I will be adding a latency histogram metrics for these BigQuery API request calls. The previous PR has written basically a side framework to count the API call latencies into a histogram and log them. But since I will be introducing a metric anyways, I suggested using this approach. So we wouldn't essentially have two code paths to count the latency values for logging, and for the system metric.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r505806435
##########
File path: sdks/python/apache_beam/metrics/cells.py
##########
@@ -238,6 +250,57 @@ def to_runner_api_monitoring_info(self, name, transform_id):
ptransform=transform_id)
+class HistogramCell(MetricCell):
+ """For internal use only; no backwards-compatibility guarantees.
+
+ Tracks the current value and delta for a histogram metric.
+
+ Each cell tracks the state of a metric independently per context per bundle.
+ Therefore, each metric has a different cell in each bundle, that is later
+ aggregated.
+
+ This class is thread safe since underlying histogram object is thread safe.
+ """
+ def __init__(self, bucket_type):
+ self._bucket_type = bucket_type
+ self.data = HistogramAggregator(bucket_type).identity_element()
+
+ def reset(self):
+ self.data = HistogramAggregator(self._bucket_type).identity_element()
+
+ def combine(self, other):
+ # type: (HistogramCell) -> HistogramCell
+ result = HistogramCell(self._bucket_type)
+ result.data = self.data.combine(other.data)
+ return result
+
+ def update(self, value):
+ self.data.histogram.record(value)
+
+ def get_cumulative(self):
+ # type: () -> HistogramData
+ return self.data.get_cumulative()
+
+ def to_runner_api_monitoring_info(self, name, transform_id):
+ return None
+
+
+class HistogramCellFactory(MetricCellFactory):
Review comment:
`MetricCellFactory` creates a customized factory instance for creating `MetricCell`. Currently, `MetricCell` is created from `Type[MetricCell]` and cannot be parameterized. `HistogramCell` needs to support multiple bucket types with their own parameters so creating from singleton type objects didn't work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/6f58212d42ce387a14b6d7738977cf1380a655aa?el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `83.73%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.48% -0.01%
==========================================
Files 454 455 +1
Lines 54797 55028 +231
==========================================
+ Hits 45202 45389 +187
- Misses 9595 9639 +44
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <75.00%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `93.43% <91.66%> (-0.86%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `82.70% <100.00%> (+2.86%)` | :arrow_up: |
| [sdks/python/apache\_beam/testing/test\_utils.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdGVzdGluZy90ZXN0X3V0aWxzLnB5) | `100.00% <100.00%> (ø)` | |
| ... and [19 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...cd13945](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r501954641
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
We do plan on building a few system metrics for GCP IO API request call latency which use histograms. Though, it doesn't need to use the user metric framework itself, its a reasonable way to implement it to use a lot of the same primitives.
I asked @ihji to use the metric framework for the logger, as I will be adding a latency histogram metrics for these BigQuery API request calls. The previous PR has written basically a side framework to count the API call latencies into a histogram and log them. But since I will be introducing a metric anyways, I suggested using this approach. So we wouldn't essentially have two code paths to count the latency values for logging, and for the system metric.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r501390668
##########
File path: sdks/python/apache_beam/metrics/cells.py
##########
@@ -238,6 +250,57 @@ def to_runner_api_monitoring_info(self, name, transform_id):
ptransform=transform_id)
+class HistogramCell(MetricCell):
+ """For internal use only; no backwards-compatibility guarantees.
+
+ Tracks the current value and delta for a histogram metric.
+
+ Each cell tracks the state of a metric independently per context per bundle.
+ Therefore, each metric has a different cell in each bundle, that is later
+ aggregated.
+
+ This class is thread safe since underlying histogram object is thread safe.
+ """
+ def __init__(self, bucket_type):
+ self._bucket_type = bucket_type
+ self.data = HistogramAggregator(bucket_type).identity_element()
+
+ def reset(self):
+ self.data = HistogramAggregator(self._bucket_type).identity_element()
+
+ def combine(self, other):
+ # type: (HistogramCell) -> HistogramCell
+ result = HistogramCell(self._bucket_type)
+ result.data = self.data.combine(other.data)
+ return result
+
+ def update(self, value):
+ self.data.histogram.record(value)
+
+ def get_cumulative(self):
+ # type: () -> HistogramData
+ return self.data.get_cumulative()
+
+ def to_runner_api_monitoring_info(self, name, transform_id):
Review comment:
Might be good to ensure this isn't called. Can we throw not implemented?
Or if that breaks other code, at least comment why this isn't populated yet.
##########
File path: sdks/python/apache_beam/io/gcp/bigquery.py
##########
@@ -1102,7 +1096,7 @@ def __init__(
retry_strategy=None,
additional_bq_parameters=None,
ignore_insert_ids=False,
- latency_logging_frequency_sec=None):
+ streaming_api_logging_frequency_sec=None):
Review comment:
Can this be obtained without plumbing to the ctor. I.e. as a pipeline option accessed directly, or constant in this file?
##########
File path: sdks/python/apache_beam/metrics/metric.py
##########
@@ -293,3 +339,49 @@ def with_steps(self, steps):
self._steps.update(steps)
return self
+
+
+class MetricLogger(object):
Review comment:
Can you test that this class works if you change the current metric environment. We should make sure that all instances of the metic on all MetricEnvironments will be logged. AS the DelegatingMetric's value changes based on the MetricEnvironment it is set in. I suspect what you are doing works fine, as long as long as you keep a separate timer for each MetricEnvironment. I.e. imagine two transforms with a metirc in two separate MetricEnvironments. If they are sharing the same time in the same MetricLogger, it may log for one metric environment, but not the other one, when it tries to log.
Note:
- Every time a new bundle is processed, a new MetricEnvironment is created, which will clear the metrics.
- Separate transforms, or code running in separate threads will store metrics in separate MetricEnvironments
- DelegatingMetrics always use the current MetricEnvironment to store a metric, or get a metric value (This is a thread local variable which is reset when a new bundle is processed, new transform is executing, etc.).
##########
File path: sdks/python/apache_beam/metrics/cells.py
##########
@@ -238,6 +250,57 @@ def to_runner_api_monitoring_info(self, name, transform_id):
ptransform=transform_id)
+class HistogramCell(MetricCell):
+ """For internal use only; no backwards-compatibility guarantees.
+
+ Tracks the current value and delta for a histogram metric.
+
+ Each cell tracks the state of a metric independently per context per bundle.
+ Therefore, each metric has a different cell in each bundle, that is later
+ aggregated.
+
+ This class is thread safe since underlying histogram object is thread safe.
+ """
+ def __init__(self, bucket_type):
+ self._bucket_type = bucket_type
+ self.data = HistogramAggregator(bucket_type).identity_element()
+
+ def reset(self):
+ self.data = HistogramAggregator(self._bucket_type).identity_element()
+
+ def combine(self, other):
+ # type: (HistogramCell) -> HistogramCell
+ result = HistogramCell(self._bucket_type)
+ result.data = self.data.combine(other.data)
+ return result
+
+ def update(self, value):
+ self.data.histogram.record(value)
+
+ def get_cumulative(self):
+ # type: () -> HistogramData
+ return self.data.get_cumulative()
+
+ def to_runner_api_monitoring_info(self, name, transform_id):
+ return None
+
+
+class HistogramCellFactory(MetricCellFactory):
Review comment:
Is this class necessary? It seems like it completely defers to HistogramCell, regardless of bucket_type or any parameters. Can it be removed. Do any of the other Cell classes have a Factory like this? I didn't notice this elsewhere.
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
We should make this clear that this isn't fully supported as a user metric type in the SDK, or in any Runners. We may need to relocate this class from Metrics.histogram to somewhere else i.e. InternalMetrics.histogram
@pabloem any ideas what we ought to do here? This metric isn't fully implement but the API allows us to log the metric locally.
##########
File path: sdks/python/apache_beam/metrics/cells.py
##########
@@ -238,6 +250,57 @@ def to_runner_api_monitoring_info(self, name, transform_id):
ptransform=transform_id)
+class HistogramCell(MetricCell):
+ """For internal use only; no backwards-compatibility guarantees.
+
+ Tracks the current value and delta for a histogram metric.
+
+ Each cell tracks the state of a metric independently per context per bundle.
+ Therefore, each metric has a different cell in each bundle, that is later
+ aggregated.
+
+ This class is thread safe since underlying histogram object is thread safe.
+ """
+ def __init__(self, bucket_type):
+ self._bucket_type = bucket_type
+ self.data = HistogramAggregator(bucket_type).identity_element()
+
+ def reset(self):
+ self.data = HistogramAggregator(self._bucket_type).identity_element()
+
+ def combine(self, other):
+ # type: (HistogramCell) -> HistogramCell
+ result = HistogramCell(self._bucket_type)
+ result.data = self.data.combine(other.data)
+ return result
+
+ def update(self, value):
+ self.data.histogram.record(value)
+
+ def get_cumulative(self):
+ # type: () -> HistogramData
+ return self.data.get_cumulative()
+
+ def to_runner_api_monitoring_info(self, name, transform_id):
+ return None
+
+
+class HistogramCellFactory(MetricCellFactory):
Review comment:
Additionally, its not clear to me that MetricCellFactory is necessary
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r505015101
##########
File path: sdks/python/apache_beam/metrics/metric.py
##########
@@ -139,6 +169,22 @@ def __init__(self, metric_name):
super(Metrics.DelegatingGauge, self).__init__(metric_name)
self.set = MetricUpdater(cells.GaugeCell, metric_name) # type: ignore[assignment]
+ class DelegatingHistogram(Histogram):
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji merged pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ihji merged pull request #13017:
URL: https://github.com/apache/beam/pull/13017
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r501954641
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
We do plan on building a few system metrics for GCP IO API request call latency which use histograms. Though, it doesn't need to use the user metric framework itself, its a reasonable way to implement it to use a lot of the same primitives.
I asked @ihji to use the metric framework for the logger, as I will be adding a latency histogram metrics for these BigQuery API request calls. The previous PR has written basically a side framework to count the API call latencies into a histogram and log them. But since I will be introducing a metric anyways, I suggested using this approach. So we wouldn't essentially have two code paths to count the metric for logging, and for the system metric.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/29787b38b594e29428adaf230b45f9b33e24fa66?el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `72.72%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.45% -0.04%
==========================================
Files 454 455 +1
Lines 54813 55028 +215
==========================================
+ Hits 45215 45375 +160
- Misses 9598 9653 +55
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `81.02% <44.44%> (-13.27%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <66.66%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [sdks/python/apache\_beam/utils/retry.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvcmV0cnkucHk=) | `86.11% <0.00%> (-0.93%)` | :arrow_down: |
| [...ks/python/apache\_beam/runners/worker/statecache.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvc3RhdGVjYWNoZS5weQ==) | `96.18% <0.00%> (-0.54%)` | :arrow_down: |
| ... and [13 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...486f4d3](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r508841422
##########
File path: sdks/python/apache_beam/internal/metrics/metric.py
##########
@@ -0,0 +1,138 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""
+Metrics API classes for internal use only.
+
+Users should use apache_beam.metrics.metric package instead.
+
+For internal use only. No backwards compatibility guarantees.
+"""
+# pytype: skip-file
+# mypy: disallow-untyped-defs
+
+from __future__ import absolute_import
+
+import datetime
+import logging
+import threading
+import time
+from builtins import object
+from typing import TYPE_CHECKING
+from typing import Dict
+from typing import Optional
+from typing import Type
+from typing import Union
+
+from apache_beam.internal.metrics.cells import HistogramCellFactory
+from apache_beam.metrics.execution import MetricUpdater
+from apache_beam.metrics.metric import Metrics as UserMetrics
+from apache_beam.metrics.metricbase import Histogram
+from apache_beam.metrics.metricbase import MetricName
+
+if TYPE_CHECKING:
+ from apache_beam.metrics.cells import MetricCell
+ from apache_beam.metrics.cells import MetricCellFactory
+ from apache_beam.utils.histogram import BucketType
+
+__all__ = ['Metrics']
+
+_LOGGER = logging.getLogger(__name__)
+
+
+class Metrics(object):
+ @staticmethod
+ def histogram(namespace, name, bucket_type, logger=None):
+ # type: (Union[Type, str], str, BucketType, Optional[MetricLogger]) -> Metrics.DelegatingHistogram
+
+ """Obtains or creates a Histogram metric.
+
+ Args:
+ namespace: A class or string that gives the namespace to a metric
+ name: A string that gives a unique name to a metric
+ bucket_type: A type of bucket used in a histogram. A subclass of
+ apache_beam.utils.histogram.BucketType
+ logger: MetricLogger for logging locally aggregated metric
+
+ Returns:
+ A Histogram object.
+ """
+ namespace = UserMetrics.get_namespace(namespace)
+ return Metrics.DelegatingHistogram(
+ MetricName(namespace, name), bucket_type, logger)
+
+ class DelegatingHistogram(Histogram):
+ """Metrics Histogram that Delegates functionality to MetricsEnvironment."""
+ def __init__(self, metric_name, bucket_type, logger):
+ # type: (MetricName, BucketType, Optional[MetricLogger]) -> None
+ super(Metrics.DelegatingHistogram, self).__init__(metric_name)
+ self.metric_name = metric_name
+ self.cell_type = HistogramCellFactory(bucket_type)
+ self.logger = logger
+ self.updater = MetricUpdater(self.cell_type, self.metric_name)
+
+ def update(self, value):
+ # type: (object) -> None
+ self.updater(value)
+ if self.logger:
+ self.logger.update(self.cell_type, self.metric_name, value)
+
+
+class MetricLogger(object):
+ """Simple object to locally aggregate and log metrics.
+
+ This class is experimental. No backwards-compatibility guarantees.
+ """
+ def __init__(self):
+ # type: () -> None
+ self._metric = dict() # type: Dict[MetricName, MetricCell]
+ self._lock = threading.Lock()
+ self._last_logging_millis = int(time.time() * 1000)
+ self.minimum_logging_frequency_msec = 180000
+
+ def update(self, cell_type, metric_name, value):
+ # type: (Union[Type[MetricCell], MetricCellFactory], MetricName, object) -> None
+ cell = self._get_metric_cell(cell_type, metric_name)
Review comment:
So justt o confirm, every time the metric is updated, regardless of which environment is set. Thee same cell will be obtained and updated here. Which aggregates everything in the process together.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji commented on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ihji commented on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-704458418
R: @ajamato @chamikaramj
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r505015346
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
pabloem commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r501944163
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
I agree with Alex. We have no plans to support this in any way by runners. It may be much simpler to just create a basic Histogram metric object that does not integrate with the whole user metrics framework.
I feel that integrating with the user metrics framework will generate confusion for users and developers looking at it in the future, as it is not intended to be a user metric.
@ihji can you talk about your motivation to integrate with the user metrics framework?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/6f58212d42ce387a14b6d7738977cf1380a655aa?el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `83.17%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.47% -0.02%
==========================================
Files 454 455 +1
Lines 54797 55028 +231
==========================================
+ Hits 45202 45385 +183
- Misses 9595 9643 +48
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <66.66%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `92.70% <88.88%> (-1.59%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `82.70% <100.00%> (+2.86%)` | :arrow_up: |
| [sdks/python/apache\_beam/testing/test\_utils.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdGVzdGluZy90ZXN0X3V0aWxzLnB5) | `100.00% <100.00%> (ø)` | |
| ... and [20 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...486f4d3](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r504283933
##########
File path: sdks/python/apache_beam/metrics/metric.py
##########
@@ -139,6 +169,22 @@ def __init__(self, metric_name):
super(Metrics.DelegatingGauge, self).__init__(metric_name)
self.set = MetricUpdater(cells.GaugeCell, metric_name) # type: ignore[assignment]
+ class DelegatingHistogram(Histogram):
Review comment:
Please move histogram related code into a new 'internal' file
sdks/python/apache_beam/internal/metrics/metric.py
Please move DelegatingHistogram to the new file
add a Metric class to the new file, and put the histogram definition there.
It can share the infrastructure and call things in this file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r504284140
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
Pablo and I discussed this. A bit more. @ihji Would you mind please moving the Metrics.histogram() into a "internal" folder. We don't want to suggest that users use this yet, if we are only using it for system/internal style metrics/monitoring.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r508886157
##########
File path: sdks/python/apache_beam/internal/metrics/metric.py
##########
@@ -0,0 +1,138 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""
+Metrics API classes for internal use only.
+
+Users should use apache_beam.metrics.metric package instead.
+
+For internal use only. No backwards compatibility guarantees.
+"""
+# pytype: skip-file
+# mypy: disallow-untyped-defs
+
+from __future__ import absolute_import
+
+import datetime
+import logging
+import threading
+import time
+from builtins import object
+from typing import TYPE_CHECKING
+from typing import Dict
+from typing import Optional
+from typing import Type
+from typing import Union
+
+from apache_beam.internal.metrics.cells import HistogramCellFactory
+from apache_beam.metrics.execution import MetricUpdater
+from apache_beam.metrics.metric import Metrics as UserMetrics
+from apache_beam.metrics.metricbase import Histogram
+from apache_beam.metrics.metricbase import MetricName
+
+if TYPE_CHECKING:
+ from apache_beam.metrics.cells import MetricCell
+ from apache_beam.metrics.cells import MetricCellFactory
+ from apache_beam.utils.histogram import BucketType
+
+__all__ = ['Metrics']
+
+_LOGGER = logging.getLogger(__name__)
+
+
+class Metrics(object):
+ @staticmethod
+ def histogram(namespace, name, bucket_type, logger=None):
+ # type: (Union[Type, str], str, BucketType, Optional[MetricLogger]) -> Metrics.DelegatingHistogram
+
+ """Obtains or creates a Histogram metric.
+
+ Args:
+ namespace: A class or string that gives the namespace to a metric
+ name: A string that gives a unique name to a metric
+ bucket_type: A type of bucket used in a histogram. A subclass of
+ apache_beam.utils.histogram.BucketType
+ logger: MetricLogger for logging locally aggregated metric
+
+ Returns:
+ A Histogram object.
+ """
+ namespace = UserMetrics.get_namespace(namespace)
+ return Metrics.DelegatingHistogram(
+ MetricName(namespace, name), bucket_type, logger)
+
+ class DelegatingHistogram(Histogram):
+ """Metrics Histogram that Delegates functionality to MetricsEnvironment."""
+ def __init__(self, metric_name, bucket_type, logger):
+ # type: (MetricName, BucketType, Optional[MetricLogger]) -> None
+ super(Metrics.DelegatingHistogram, self).__init__(metric_name)
+ self.metric_name = metric_name
+ self.cell_type = HistogramCellFactory(bucket_type)
+ self.logger = logger
+ self.updater = MetricUpdater(self.cell_type, self.metric_name)
+
+ def update(self, value):
+ # type: (object) -> None
+ self.updater(value)
+ if self.logger:
+ self.logger.update(self.cell_type, self.metric_name, value)
+
+
+class MetricLogger(object):
+ """Simple object to locally aggregate and log metrics.
+
+ This class is experimental. No backwards-compatibility guarantees.
+ """
+ def __init__(self):
+ # type: () -> None
+ self._metric = dict() # type: Dict[MetricName, MetricCell]
+ self._lock = threading.Lock()
+ self._last_logging_millis = int(time.time() * 1000)
+ self.minimum_logging_frequency_msec = 180000
+
+ def update(self, cell_type, metric_name, value):
+ # type: (Union[Type[MetricCell], MetricCellFactory], MetricName, object) -> None
+ cell = self._get_metric_cell(cell_type, metric_name)
Review comment:
True. The reason was for streaming pipelines the bundle size is too small so that per bundle metrics are not really helpful especially for statistics like histogram percentiles. `MetricLogger` could be considered as a process-wide shared metric container.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r508831092
##########
File path: sdks/python/apache_beam/metrics/metric.py
##########
@@ -293,3 +339,49 @@ def with_steps(self, steps):
self._steps.update(steps)
return self
+
+
+class MetricLogger(object):
Review comment:
`MetricLogger` stores separate metric values from `MetricEnvironment` for reporting a single aggregated result to the logger. In this PR, it's used for keeping process-wide API latency histogram values and periodically logging it. So I think changing `MetricEnvironment` doesn't have any effect here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r508839974
##########
File path: sdks/python/apache_beam/metrics/metric.py
##########
@@ -293,3 +339,49 @@ def with_steps(self, steps):
self._steps.update(steps)
return self
+
+
+class MetricLogger(object):
Review comment:
Oh okay thanks, this clarifies it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] commented on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/29787b38b594e29428adaf230b45f9b33e24fa66?el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `72.72%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.45% -0.04%
==========================================
Files 454 455 +1
Lines 54813 55028 +215
==========================================
+ Hits 45215 45375 +160
- Misses 9598 9653 +55
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `81.02% <44.44%> (-13.27%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <66.66%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [sdks/python/apache\_beam/utils/retry.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvcmV0cnkucHk=) | `86.11% <0.00%> (-0.93%)` | :arrow_down: |
| [...ks/python/apache\_beam/runners/worker/statecache.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvc3RhdGVjYWNoZS5weQ==) | `96.18% <0.00%> (-0.54%)` | :arrow_down: |
| ... and [13 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...764196c](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/6f58212d42ce387a14b6d7738977cf1380a655aa?el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `83.73%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.48% -0.01%
==========================================
Files 454 455 +1
Lines 54797 55028 +231
==========================================
+ Hits 45202 45389 +187
- Misses 9595 9639 +44
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <75.00%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `93.43% <91.66%> (-0.86%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `82.70% <100.00%> (+2.86%)` | :arrow_up: |
| [sdks/python/apache\_beam/testing/test\_utils.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdGVzdGluZy90ZXN0X3V0aWxzLnB5) | `100.00% <100.00%> (ø)` | |
| ... and [19 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...2da9852](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r507892565
##########
File path: sdks/python/apache_beam/internal/metrics/cells.py
##########
@@ -0,0 +1,190 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""
+This file contains internal metric cell classes. A metric cell is used to
+accumulate in-memory changes to a metric. It represents a specific metric
+in a single context.
+
+For internal use only. No backwards compatibility guarantees.
+"""
+
+# pytype: skip-file
+
+from __future__ import absolute_import
+from __future__ import division
+
+from builtins import object
+from typing import TYPE_CHECKING
+from typing import Optional
+
+from apache_beam.metrics.cells import MetricAggregator
+from apache_beam.metrics.cells import MetricCell
+from apache_beam.metrics.cells import MetricCellFactory
+from apache_beam.utils.histogram import Histogram
+
+if TYPE_CHECKING:
+ from apache_beam.utils.histogram import BucketType
+
+
+class HistogramCell(MetricCell):
+ """For internal use only; no backwards-compatibility guarantees.
+
+ Tracks the current value and delta for a histogram metric.
+
+ Each cell tracks the state of a metric independently per context per bundle.
+ Therefore, each metric has a different cell in each bundle, that is later
+ aggregated.
+
+ This class is thread safe since underlying histogram object is thread safe.
+ """
+ def __init__(self, bucket_type):
+ self._bucket_type = bucket_type
+ self.data = HistogramAggregator(bucket_type).identity_element()
+
+ def reset(self):
+ self.data = HistogramAggregator(self._bucket_type).identity_element()
+
+ def combine(self, other):
+ # type: (HistogramCell) -> HistogramCell
+ result = HistogramCell(self._bucket_type)
+ result.data = self.data.combine(other.data)
+ return result
+
+ def update(self, value):
+ self.data.histogram.record(value)
+
+ def get_cumulative(self):
+ # type: () -> HistogramData
+ return self.data.get_cumulative()
+
+ def to_runner_api_monitoring_info(self, name, transform_id):
+ # Histogram metric is currently worker-local and internal
+ # use only. This method should be implemented when runners
+ # support Histogram metric reporting.
+ return None
+
+
+class HistogramCellFactory(MetricCellFactory):
+ def __init__(self, bucket_type):
+ self._bucket_type = bucket_type
+
+ def __call__(self):
+ return HistogramCell(self._bucket_type)
+
+ def __eq__(self, other):
+ if not isinstance(other, HistogramCellFactory):
+ return False
+ return self._bucket_type == other._bucket_type
+
+ def __hash__(self):
+ return hash(self._bucket_type)
+
+
+class HistogramResult(object):
+ def __init__(self, data):
+ # type: (HistogramData) -> None
+ self.data = data
+
+ def __eq__(self, other):
+ if isinstance(other, HistogramResult):
+ return self.data == other.data
+ else:
+ return False
+
+ def __hash__(self):
+ return hash(self.data)
+
+ def __ne__(self, other):
+ # TODO(BEAM-5949): Needed for Python 2 compatibility.
+ return not self == other
+
+ def __repr__(self):
+ return '<HistogramResult({})>'.format(
+ self.data.histogram.get_percentile_info())
+
+ @property
+ def p99(self):
+ return self.data.histogram.p99()
+
+ @property
+ def p95(self):
+ return self.data.histogram.p95()
+
+ @property
+ def p90(self):
+ return self.data.histogram.p90()
+
+
+class HistogramData(object):
+ """For internal use only; no backwards-compatibility guarantees.
+
+ The data structure that holds data about a histogram metric.
+
+ This object is not thread safe, so it's not supposed to be modified
+ by other than the HistogramCell that contains it.
Review comment:
*modified outside the HistogramCell...
##########
File path: sdks/python/apache_beam/metrics/metric.py
##########
@@ -293,3 +339,49 @@ def with_steps(self, steps):
self._steps.update(steps)
return self
+
+
+class MetricLogger(object):
Review comment:
Do you know the behaviour here of your change? Like I said, it looks like you will only be logging the state of a metric in the current MetricContainer. Rather then all of the metrics in separate MetricContainers aggregated together into a single value.
Is there a guarantee that you will log once for every metric container? (I don;'t think so because of the timing logic)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r501954641
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
We do plan on building a few system metrics for GCP IO API request call latency which use histograms. Though, it doesn't need to use the user metric framework itself, its a reasonable way to implement it to use a lot of the same primitives.
I asked @ihji to use the metric framework for the logger, as I will be adding a latency histogram metrics for these BigQuery API request calls.
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
We do plan on building a few system metrics for GCP IO API request call latency which use histograms. Though, it doesn't need to use the user metric framework itself, its a reasonable way to implement it to use a lot of the same primitives.
I asked @ihji to use the metric framework for the logger, as I will be adding a latency histogram metrics for these BigQuery API request calls. The previous PR has written basically a side framework to count the API call latencies into a histogram and log them. But since I will be introducing a metric anyways, I suggested using this approach.
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
We do plan on building a few system metrics for GCP IO API request call latency which use histograms. Though, it doesn't need to use the user metric framework itself, its a reasonable way to implement it to use a lot of the same primitives.
I asked @ihji to use the metric framework for the logger, as I will be adding a latency histogram metrics for these BigQuery API request calls. The previous PR has written basically a side framework to count the API call latencies into a histogram and log them. But since I will be introducing a metric anyways, I suggested using this approach. So we wouldn't essentially have two code paths to count the metric for logging, and for the system metric.
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
We do plan on building a few system metrics for GCP IO API request call latency which use histograms. Though, it doesn't need to use the user metric framework itself, its a reasonable way to implement it to use a lot of the same primitives.
I asked @ihji to use the metric framework for the logger, as I will be adding a latency histogram metrics for these BigQuery API request calls. The previous PR has written basically a side framework to count the API call latencies into a histogram and log them. But since I will be introducing a metric anyways, I suggested using this approach. So we wouldn't essentially have two code paths to count the latency values for logging, and for the system metric.
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
We do plan on building a few system metrics for GCP IO API request call latency which use histograms. Though, it doesn't need to use the user metric framework itself, its a reasonable way to implement it to use a lot of the same techniques.
I asked @ihji to use the metric framework for the logger, as I will be adding a latency histogram metrics for these BigQuery API request calls. The previous PR has written basically a side framework to count the API call latencies into a histogram and log them. But since I will be introducing a metric anyways, I suggested using this approach. So we wouldn't essentially have two code paths to count the latency values for logging, and for the system metric.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r501954641
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
We do plan on building a few system metrics for GCP IO API request call latency which use histograms. Though, it doesn't need to use the user metric framework itself, its a reasonable way to implement it to use a lot of the same primitives.
I asked @ihji to use the metric framework for the logger, as I will be adding a latency histogram metrics for these BigQuery API request calls. The previous PR has written basically a side framework to count the API call latencies into a histogram and log them. But since I will be introducing a metric anyways, I suggested using this approach.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ajamato commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ajamato commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r501954641
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
We do plan on building a few system metrics for GCP IO API request call latency which use histograms. Though, it doesn't need to use the user metric framework itself, its a reasonable way to implement it to use a lot of the same primitives.
I asked @ihji to use the metric framework for the logger, as I will be adding a latency histogram metrics for these BigQuery API request calls.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r505014935
##########
File path: sdks/python/apache_beam/metrics/cells.py
##########
@@ -238,6 +250,57 @@ def to_runner_api_monitoring_info(self, name, transform_id):
ptransform=transform_id)
+class HistogramCell(MetricCell):
+ """For internal use only; no backwards-compatibility guarantees.
+
+ Tracks the current value and delta for a histogram metric.
+
+ Each cell tracks the state of a metric independently per context per bundle.
+ Therefore, each metric has a different cell in each bundle, that is later
+ aggregated.
+
+ This class is thread safe since underlying histogram object is thread safe.
+ """
+ def __init__(self, bucket_type):
+ self._bucket_type = bucket_type
+ self.data = HistogramAggregator(bucket_type).identity_element()
+
+ def reset(self):
+ self.data = HistogramAggregator(self._bucket_type).identity_element()
+
+ def combine(self, other):
+ # type: (HistogramCell) -> HistogramCell
+ result = HistogramCell(self._bucket_type)
+ result.data = self.data.combine(other.data)
+ return result
+
+ def update(self, value):
+ self.data.histogram.record(value)
+
+ def get_cumulative(self):
+ # type: () -> HistogramData
+ return self.data.get_cumulative()
+
+ def to_runner_api_monitoring_info(self, name, transform_id):
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/6f58212d42ce387a14b6d7738977cf1380a655aa?el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `83.73%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.48% -0.01%
==========================================
Files 454 455 +1
Lines 54797 55028 +231
==========================================
+ Hits 45202 45389 +187
- Misses 9595 9639 +44
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <75.00%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `93.43% <91.66%> (-0.86%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `82.70% <100.00%> (+2.86%)` | :arrow_up: |
| [sdks/python/apache\_beam/testing/test\_utils.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdGVzdGluZy90ZXN0X3V0aWxzLnB5) | `100.00% <100.00%> (ø)` | |
| ... and [19 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...6bc49b1](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/6f58212d42ce387a14b6d7738977cf1380a655aa?el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `83.73%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.48% -0.01%
==========================================
Files 454 455 +1
Lines 54797 55028 +231
==========================================
+ Hits 45202 45389 +187
- Misses 9595 9639 +44
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <75.00%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `93.43% <91.66%> (-0.86%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `82.70% <100.00%> (+2.86%)` | :arrow_up: |
| [sdks/python/apache\_beam/testing/test\_utils.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdGVzdGluZy90ZXN0X3V0aWxzLnB5) | `100.00% <100.00%> (ø)` | |
| ... and [19 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...b165a8c](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
pabloem commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r501944163
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -271,6 +276,11 @@ def __init__(self, client=None):
# randomized prefix for row IDs.
self._row_id_prefix = '' if client else uuid.uuid4()
self._temporary_table_suffix = uuid.uuid4().hex
+ self._latency_histogram_metric = Metrics.histogram(
Review comment:
I agree with Alex. We have no plans to support this in any way by runners. It may be much simpler to just create a basic Histogram metric object that does not integrate with the whole user metrics framework.
I feel that integrating with the user metrics framework will generate confusion for users and developers looking at it in the future, as it is not intended to be a user metric.
@ihji can you talk about your motivation to integrate with the user metrics framework?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/6f58212d42ce387a14b6d7738977cf1380a655aa?el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `83.17%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.47% -0.02%
==========================================
Files 454 455 +1
Lines 54797 55028 +231
==========================================
+ Hits 45202 45385 +183
- Misses 9595 9643 +48
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <66.66%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `92.70% <88.88%> (-1.59%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `82.70% <100.00%> (+2.86%)` | :arrow_up: |
| [sdks/python/apache\_beam/testing/test\_utils.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdGVzdGluZy90ZXN0X3V0aWxzLnB5) | `100.00% <100.00%> (ø)` | |
| ... and [20 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...486f4d3](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/6f58212d42ce387a14b6d7738977cf1380a655aa?el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `83.73%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.48% -0.01%
==========================================
Files 454 455 +1
Lines 54797 55028 +231
==========================================
+ Hits 45202 45389 +187
- Misses 9595 9639 +44
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <75.00%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `93.43% <91.66%> (-0.86%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `82.70% <100.00%> (+2.86%)` | :arrow_up: |
| [sdks/python/apache\_beam/testing/test\_utils.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdGVzdGluZy90ZXN0X3V0aWxzLnB5) | `100.00% <100.00%> (ø)` | |
| ... and [19 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...1666dde](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/6f58212d42ce387a14b6d7738977cf1380a655aa?el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `83.73%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.48% -0.01%
==========================================
Files 454 455 +1
Lines 54797 55028 +231
==========================================
+ Hits 45202 45389 +187
- Misses 9595 9639 +44
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <75.00%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `93.43% <91.66%> (-0.86%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `82.70% <100.00%> (+2.86%)` | :arrow_up: |
| [sdks/python/apache\_beam/testing/test\_utils.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdGVzdGluZy90ZXN0X3V0aWxzLnB5) | `100.00% <100.00%> (ø)` | |
| ... and [19 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...d284779](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r505013809
##########
File path: sdks/python/apache_beam/io/gcp/bigquery.py
##########
@@ -1102,7 +1096,7 @@ def __init__(
retry_strategy=None,
additional_bq_parameters=None,
ignore_insert_ids=False,
- latency_logging_frequency_sec=None):
+ streaming_api_logging_frequency_sec=None):
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/6f58212d42ce387a14b6d7738977cf1380a655aa?el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `83.73%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.48% -0.01%
==========================================
Files 454 455 +1
Lines 54797 55028 +231
==========================================
+ Hits 45202 45389 +187
- Misses 9595 9639 +44
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <75.00%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `93.43% <91.66%> (-0.86%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `82.70% <100.00%> (+2.86%)` | :arrow_up: |
| [sdks/python/apache\_beam/testing/test\_utils.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdGVzdGluZy90ZXN0X3V0aWxzLnB5) | `100.00% <100.00%> (ø)` | |
| ... and [19 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...ce5d5a4](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13017:
URL: https://github.com/apache/beam/pull/13017#issuecomment-703976988
# [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=h1) Report
> Merging [#13017](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=desc) into [master](https://codecov.io/gh/apache/beam/commit/29787b38b594e29428adaf230b45f9b33e24fa66?el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `72.72%`.
[](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13017 +/- ##
==========================================
- Coverage 82.48% 82.45% -0.04%
==========================================
Files 454 455 +1
Lines 54813 55028 +215
==========================================
+ Hits 45215 45375 +160
- Misses 9598 9653 +55
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...dks/python/apache\_beam/options/pipeline\_options.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vb3B0aW9ucy9waXBlbGluZV9vcHRpb25zLnB5) | `93.76% <ø> (ø)` | |
| [sdks/python/apache\_beam/utils/histogram.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaGlzdG9ncmFtLnB5) | `81.02% <44.44%> (-13.27%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/execution.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9leGVjdXRpb24ucHk=) | `86.71% <50.00%> (-0.69%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metricbase.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWNiYXNlLnB5) | `86.48% <66.66%> (-1.75%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/cells.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9jZWxscy5weQ==) | `77.91% <69.87%> (-2.83%)` | :arrow_down: |
| [sdks/python/apache\_beam/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWV0cmljcy9tZXRyaWMucHk=) | `94.41% <92.15%> (-0.90%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5LnB5) | `79.03% <100.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `88.43% <100.00%> (+0.07%)` | :arrow_up: |
| [sdks/python/apache\_beam/utils/retry.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvcmV0cnkucHk=) | `86.11% <0.00%> (-0.93%)` | :arrow_down: |
| [...ks/python/apache\_beam/runners/worker/statecache.py](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvc3RhdGVjYWNoZS5weQ==) | `96.18% <0.00%> (-0.54%)` | :arrow_down: |
| ... and [13 more](https://codecov.io/gh/apache/beam/pull/13017/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=footer). Last update [cfe8109...764196c](https://codecov.io/gh/apache/beam/pull/13017?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ihji commented on a change in pull request #13017: [BEAM-11018] Use metric for Python BigQuery streaming insert API late…
Posted by GitBox <gi...@apache.org>.
ihji commented on a change in pull request #13017:
URL: https://github.com/apache/beam/pull/13017#discussion_r508925840
##########
File path: sdks/python/apache_beam/internal/metrics/cells.py
##########
@@ -0,0 +1,190 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+"""
+This file contains internal metric cell classes. A metric cell is used to
+accumulate in-memory changes to a metric. It represents a specific metric
+in a single context.
+
+For internal use only. No backwards compatibility guarantees.
+"""
+
+# pytype: skip-file
+
+from __future__ import absolute_import
+from __future__ import division
+
+from builtins import object
+from typing import TYPE_CHECKING
+from typing import Optional
+
+from apache_beam.metrics.cells import MetricAggregator
+from apache_beam.metrics.cells import MetricCell
+from apache_beam.metrics.cells import MetricCellFactory
+from apache_beam.utils.histogram import Histogram
+
+if TYPE_CHECKING:
+ from apache_beam.utils.histogram import BucketType
+
+
+class HistogramCell(MetricCell):
+ """For internal use only; no backwards-compatibility guarantees.
+
+ Tracks the current value and delta for a histogram metric.
+
+ Each cell tracks the state of a metric independently per context per bundle.
+ Therefore, each metric has a different cell in each bundle, that is later
+ aggregated.
+
+ This class is thread safe since underlying histogram object is thread safe.
+ """
+ def __init__(self, bucket_type):
+ self._bucket_type = bucket_type
+ self.data = HistogramAggregator(bucket_type).identity_element()
+
+ def reset(self):
+ self.data = HistogramAggregator(self._bucket_type).identity_element()
+
+ def combine(self, other):
+ # type: (HistogramCell) -> HistogramCell
+ result = HistogramCell(self._bucket_type)
+ result.data = self.data.combine(other.data)
+ return result
+
+ def update(self, value):
+ self.data.histogram.record(value)
+
+ def get_cumulative(self):
+ # type: () -> HistogramData
+ return self.data.get_cumulative()
+
+ def to_runner_api_monitoring_info(self, name, transform_id):
+ # Histogram metric is currently worker-local and internal
+ # use only. This method should be implemented when runners
+ # support Histogram metric reporting.
+ return None
+
+
+class HistogramCellFactory(MetricCellFactory):
+ def __init__(self, bucket_type):
+ self._bucket_type = bucket_type
+
+ def __call__(self):
+ return HistogramCell(self._bucket_type)
+
+ def __eq__(self, other):
+ if not isinstance(other, HistogramCellFactory):
+ return False
+ return self._bucket_type == other._bucket_type
+
+ def __hash__(self):
+ return hash(self._bucket_type)
+
+
+class HistogramResult(object):
+ def __init__(self, data):
+ # type: (HistogramData) -> None
+ self.data = data
+
+ def __eq__(self, other):
+ if isinstance(other, HistogramResult):
+ return self.data == other.data
+ else:
+ return False
+
+ def __hash__(self):
+ return hash(self.data)
+
+ def __ne__(self, other):
+ # TODO(BEAM-5949): Needed for Python 2 compatibility.
+ return not self == other
+
+ def __repr__(self):
+ return '<HistogramResult({})>'.format(
+ self.data.histogram.get_percentile_info())
+
+ @property
+ def p99(self):
+ return self.data.histogram.p99()
+
+ @property
+ def p95(self):
+ return self.data.histogram.p95()
+
+ @property
+ def p90(self):
+ return self.data.histogram.p90()
+
+
+class HistogramData(object):
+ """For internal use only; no backwards-compatibility guarantees.
+
+ The data structure that holds data about a histogram metric.
+
+ This object is not thread safe, so it's not supposed to be modified
+ by other than the HistogramCell that contains it.
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org