Revised Monitoring example #96
Conversation
exporter/metric/README.md
Outdated
| ) | ||
|
|
||
| // Initialize exporter option. | ||
| opts := []mexporter.Option{} | ||
| projectID := os.Getenv("GOOGLE_CLOUD_PROJECT") |
There was a problem hiding this comment.
As I commented in #95 (comment), this shouldn't be necessary when the application is running in GCP environment.
So I would prefer to omit this in the initial example, and then add a separate short paragraph describing how to set / override project ID if needed.
There was a problem hiding this comment.
OK. Will revise. Would you like me to make the change to the Trace README example too to keep them consistent? if so, I'll submit the Trace change as a separate PR.
exporter/metric/README.md
Outdated
| mexporter.WithProjectID(projectID), | ||
| } | ||
| popts:= []push.Option{ | ||
| push.WithResource( |
There was a problem hiding this comment.
Not sure if this is useful for the minimal example. Maybe we should fix the InstallNewPipeline to allow null popts instead? Alternatively, just make an empty slice.
There was a problem hiding this comment.
Apologies if this is an obvious question, but what does having a resource do here? My understanding is that if the application is running on GCP the resource is autodetected, otherwise the labels in the resource would need to fully match and populate a GCP monitored resource here. If it doesn't match anything, it goes to "global". That would be the case for the labels below.
If the example is for something where the resource can't be autodetected, then it'd be better to guide users toward generic_node or generic_task. Support would need to be added in the exporter for those but "global" as a monitored resource is not recommended for performance and scalability reasons.
There was a problem hiding this comment.
Yes, supporting nil is best ;-)
If I use v0.11.0 then an empty slice generates an error:
2020/09/11 10:04:55 Error during exporting TimeSeries: currently the aggregator is not supported: &{{0 0} 123 [123]}
2020/09/11 10:04:55 rpc error: code = InvalidArgument desc = Request was missing field timeSeries.
If I use v0.11.1-0.20200910093037-ef5e24b7fcfc then an empty slice works. Will change to proactively permit.
exporter/metric/README.md
Outdated
| @@ -42,16 +51,18 @@ defer pusher.Stop() | |||
| ctx := context.Background() | |||
| meter := pusher.Provider().Meter("cloudmonitoring/example") | |||
|
|
|||
| counter := metric.Must(meter).NewInt64Counter("counter-foo") | |||
| labels := []kv.KeyValue{kv.Key("key").String("value")} | |||
| counter := metric.Must(meter).NewInt64Counter("counter.foo") | |||
There was a problem hiding this comment.
Any specific reason for replacing - with . in the metric name?
Google Cloud Monitoring is actually using / and _ as separators normally, e.g. see metrics in https://cloud.google.com/monitoring/api/metrics_kubernetes
There was a problem hiding this comment.
I'd thought I'd seen counter.foo used in another example and so inadvertently copied. Will revert.
There was a problem hiding this comment.
Reverted: 0fee3ad
The example in the Monitoring README is insufficient.
I believe this is a working example.
I removed the curious explanation of importing modules. In my experience (and editor), I can simply reference the imports in code and
go.modis revised automatically (or will be revised ongo runorgo build). So, I simplified this and made the imports exhaustive.I was unable to use
popts == niland so have used the exampleresource.New(...)approach here too.The Google Project ID is required if run off-cloud and so I've added it here. I noticed, after-the-fact, that the Trace README correctly specifies
WithProjectID(os.Getenv("GOOGLE_CLOUD_PROJECT"))and so, both READMEs are now consistent. However, neither piece of documentation explains the requirement toexport GOOGLE_CLOUD_PROJECT="..."and likely should.