gemini-code-assist[bot] commented on code in PR #35859:
URL: https://github.com/apache/beam/pull/35859#discussion_r2273834329
##########
sdks/python/apache_beam/metrics/monitoring_infos.py:
##########
@@ -367,8 +367,8 @@ def create_monitoring_info(
urn=urn, type=type_urn, labels=labels or {}, payload=payload)
except TypeError as e:
raise RuntimeError(
- f'Failed to create MonitoringInfo for urn {urn} type {type} labels ' +
- '{labels} and payload {payload}') from e
+ f'Failed to create MonitoringInfo for urn {urn} type {type_urn} ' +
+ f'labels {labels} and payload {payload}') from e
Review Comment:
This is a great fix for the error message. It correctly uses `type_urn` and
ensures all variables are interpolated.
For improved readability and to follow common Python style, you could use
implicit string literal concatenation instead of the `+` operator. This is
often considered more idiomatic.[^1]
```suggestion
f'Failed to create MonitoringInfo for urn {urn} type {type_urn} '
f'labels {labels} and payload {payload}') from e
```
#### Style Guide References
[^1]: PEP 8 recommends using implicit line joining inside parentheses,
brackets and braces for long lines. For strings, adjacent string literals are
automatically concatenated by Python, which can be used to break long strings
over multiple lines without using `+`.
##########
sdks/python/apache_beam/io/gcp/experimental/spannerio.py:
##########
@@ -356,8 +357,8 @@ def _table_metric(self, table_id, status):
labels = {
**self.base_labels,
monitoring_infos.RESOURCE_LABEL: resource,
- monitoring_infos.SPANNER_TABLE_ID: table_id
}
+ if table_id: labels[monitoring_infos.SPANNER_TABLE_ID] = table_id
Review Comment:
This is a good fix to prevent a `TypeError` when `table_id` is `None`.
I notice this same logic for creating labels is also present in
`_ReadFromPartitionFn._table_metric` and
`_WriteToSpannerDoFn._register_table_metric`. To adhere to the DRY (Don't
Repeat Yourself) principle, you might consider extracting this logic into a
common helper function.
However, since this module is being deprecated, this refactoring is not
essential. The current fix is correct and sufficient.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
