Skip to content
This repository has been archived by the owner on Dec 31, 2023. It is now read-only.

monitoring_v3.query.Query.align raises a TypeError on execution #80

Closed
plankthom opened this issue Jan 26, 2021 · 3 comments · Fixed by #83
Closed

monitoring_v3.query.Query.align raises a TypeError on execution #80

plankthom opened this issue Jan 26, 2021 · 3 comments · Fixed by #83
Assignees
Labels
api: monitoring Issues related to the googleapis/python-monitoring API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@plankthom
Copy link

plankthom commented Jan 26, 2021

Environment details

  • OS type and version: Macos 11.1
  • Python version: Python 3.8.5 (anaconda)
  • pip version: pip 21.0
  • google-cloud-monitoring version: 2.0.0

Steps to reproduce

While I can execute the query

from google.cloud.monitoring_v3.query import Query
query = Query(
     client=client,
     project=project_name,
     metric_type= 'storage.googleapis.com/storage/request_count'
).select_resources(
     bucket_name_prefix=bucket_name_prefix
).select_interval(
     end_time, start_time=end_time - report_interval
)
print(next(iter(query)))

adding an alignment aggregation fails:

from google.cloud.monitoring_v3 import Aggregation
query = query.align(
    Aggregation.Aligner.ALIGN_MAX, seconds=3600
)
print(next(iter(query)))

and raises

        params = self._build_query_params(headers_only, page_size)
>       for ts in self._client.list_time_series(**params):
E       TypeError: list_time_series() got an unexpected keyword argument 'aggregation'

.../site-packages/google/cloud/monitoring_v3/query.py:446: TypeError

Indeed, while

params["aggregation"] = types.Aggregation(

prepares an aggregation parameter,


accepts no such parameter.

Workaround

Directly use MetricServiceClient.list_time_series, with the aggregation specification within the request parameter:

request = dict(
      name=project_name,
      filter=f'metric.type = "storage.googleapis.com/storage/request_count" AND resource.label.bucket_name = starts_with("{bucket_name_prefix}")',
      interval=TimeInterval(dict(
          end_time=_to_seconds_nanos(end_time),
          start_time=_to_seconds_nanos(end_time - report_interval)
      )),
      view=ListTimeSeriesRequest.TimeSeriesView.FULL,
      aggregation=Aggregation(dict(
          per_series_aligner=Aggregation.Aligner.ALIGN_SUM,
          alignment_period=dict(seconds=3600)
      ))
  )
print(next(iter(client.list_time_series(request=request))))
@product-auto-label product-auto-label bot added the api: monitoring Issues related to the googleapis/python-monitoring API. label Jan 26, 2021
@plankthom plankthom changed the title using monitoring_v3.query.Query.align raise a TypeError using monitoring_v3.query.Query.align raises a TypeError Jan 26, 2021
@plankthom plankthom changed the title using monitoring_v3.query.Query.align raises a TypeError monitoring_v3.query.Query.align raises a TypeError on execution Jan 26, 2021
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jan 27, 2021
@busunkim96
Copy link
Contributor

Hi @plankthom,

As you've noticed, not all the request fields can be passed to the method. The list of parameters can be thought of as something like an overload. The specific set of parameters is determined by an annotation in the source protos called google.api.method_signature. See https://google.aip.dev/client-libraries/4232#method-signatures for details.

This is why name, filter, interval, and view are method parameters but aggregation is not.
https://github.com/googleapis/googleapis/blob/eabe7c0fde64b1451df6ea171b2009238b0df07c/google/monitoring/v3/metric_service.proto#L120-L125

  rpc ListTimeSeries(ListTimeSeriesRequest) returns (ListTimeSeriesResponse) {
    option (google.api.http) = {
      get: "/v3/{name=projects/*}/timeSeries"
    };
    option (google.api.method_signature) = "name,filter,interval,view";
  }

def list_time_series(
self,
request: metric_service.ListTimeSeriesRequest = None,
*,
name: str = None,
filter: str = None,
interval: common.TimeInterval = None,
view: metric_service.ListTimeSeriesRequest.TimeSeriesView = None,
retry: retries.Retry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> pagers.ListTimeSeriesPager:

@busunkim96 busunkim96 added type: question Request for information or clarification. Not an issue. and removed triage me I really want to be triaged. labels Jan 27, 2021
@busunkim96 busunkim96 self-assigned this Jan 27, 2021
@plankthom
Copy link
Author

plankthom commented Jan 29, 2021

@busunkim96

As you've noticed, not all the request fields can be passed to the method

Indeed i've noticed. The actual bug that I am reporting is that the SDK itself doesn't respect this limitation in

params["aggregation"] = types.Aggregation(

which breaks the feature documented in https://googleapis.dev/python/monitoring/latest/query.html as align method.

@busunkim96
Copy link
Contributor

Ah sorry about that, I wasn't reading carefully enough. 🤦‍♀️

It looks like iter should construct a request object with the params and then pass the request object to list_time_series.

if self._end_time is None:
raise ValueError("Query time interval not specified.")
params = self._build_query_params(headers_only, page_size)
for ts in self._client.list_time_series(**params):
yield ts

Thank you for the report!

@busunkim96 busunkim96 added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed type: question Request for information or clarification. Not an issue. labels Jan 29, 2021
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 2, 2021
gcf-merge-on-green bot pushed a commit that referenced this issue Feb 18, 2021
Construct a `ListTimeSeriesRequest` from the query params rather than passing them directly to the function. This is important when one of the params is "optional" and is not available as a kwarg on the `list_time_series()` method.


For example, `aggregation` must be set on the request object.

Fixes #80 

```py

    def list_time_series(
        self,
        request: metric_service.ListTimeSeriesRequest = None,
        *,
        name: str = None,
        filter: str = None,
        interval: common.TimeInterval = None,
        view: metric_service.ListTimeSeriesRequest.TimeSeriesView = None,
        retry: retries.Retry = gapic_v1.method.DEFAULT,
        timeout: float = None,
        metadata: Sequence[Tuple[str, str]] = (),
    ) -> pagers.ListTimeSeriesPager:
```

```py
class ListTimeSeriesRequest(proto.Message):
    r"""The ``ListTimeSeries`` request.

    Attributes:
        name (str):
            Required. The project on which to execute the request. The
            format is:

            ::

                projects/[PROJECT_ID_OR_NUMBER]
        filter (str):
            Required. A `monitoring
            filter <https://cloud.google.com/monitoring/api/v3/filters>`__
            that specifies which time series should be returned. The
            filter must specify a single metric type, and can
            additionally specify metric labels and other information.
            For example:

            ::

                metric.type = "compute.googleapis.com/instance/cpu/usage_time" AND
                    metric.labels.instance_name = "my-instance-name".
        interval (~.common.TimeInterval):
            Required. The time interval for which results
            should be returned. Only time series that
            contain data points in the specified interval
            are included in the response.
        aggregation (~.common.Aggregation):
            Specifies the alignment of data points in individual time
            series as well as how to combine the retrieved time series
            across specified labels.

            By default (if no ``aggregation`` is explicitly specified),
            the raw time series data is returned.
        order_by (str):
            Unsupported: must be left blank. The points
            in each time series are currently returned in
            reverse time order (most recent to oldest).
        view (~.metric_service.ListTimeSeriesRequest.TimeSeriesView):
            Required. Specifies which information is
            returned about the time series.
        page_size (int):
            A positive number that is the maximum number of results to
            return. If ``page_size`` is empty or more than 100,000
            results, the effective ``page_size`` is 100,000 results. If
            ``view`` is set to ``FULL``, this is the maximum number of
            ``Points`` returned. If ``view`` is set to ``HEADERS``, this
            is the maximum number of ``TimeSeries`` returned.
        page_token (str):
            If this field is not empty then it must contain the
            ``nextPageToken`` value returned by a previous call to this
            method. Using this field causes the method to return
            additional results from the previous method call.
    """

```
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: monitoring Issues related to the googleapis/python-monitoring API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants