WIP: Dimensional metric reporting #376
Conversation
|
Tony Pitale seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
| NewRelic.report_dimensional_metric(:count, "my_metric_name", 1, %{some: "attributes"}) | ||
| ``` | ||
| """ | ||
| defdelegate report_dimensional_metric(type, name, value, attributes \\ %{}), |
There was a problem hiding this comment.
I feel like there should be other functions that call this, as well. Similar to how incrementing a timeslice metric works.
| @@ -229,6 +229,17 @@ defmodule NewRelic do | |||
| defdelegate report_custom_event(type, attributes), | |||
| to: NewRelic.Harvest.Collector.CustomEvent.Harvester | |||
|
|
|||
| @doc """ | |||
| Report a Dimensional Metric. | |||
| Valid types: `:count`, `:gauge`, and `:summary`. | |||
There was a problem hiding this comment.
Probably should add some docs (or a link) on when to use each of the types.
lib/new_relic/harvest/telemetry_sdk/dimensional_metrics/harvester.ex
Outdated
Show resolved
Hide resolved
|
|
||
| # API | ||
|
|
||
| @spec report_dimensional_metric(:count | :gauge | :summary, atom() | binary(), any, map()) :: |
There was a problem hiding this comment.
Should I remove the typespec?
There was a problem hiding this comment.
I feel like a pretty detailed spec would be worth it on the function inside NewRelic module, I'd go beyond what's here to help folks understand what the value should be for the various types
| ] | ||
| end | ||
|
|
||
| defp common(%{start_time_ms: start_time_ms}) do |
There was a problem hiding this comment.
I made the start time the common timestamp for any metrics in this harvest cycle. I don't think there is any accuracy below (or even near to) 5 seconds when querying.
| assert common["timestamp"] > 0 | ||
|
|
||
| assert length(metrics) == 2 | ||
| [metric1, metric2] = metrics |
There was a problem hiding this comment.
This points out that the metrics are coming in reverse order because that's how they're added. Not sure if it is worth swapping the order, or reversing the metrics at gather time.
|
|
||
| @spec report_dimensional_metric(:count | :gauge | :summary, atom() | binary(), any, map()) :: | ||
| :ok | ||
| def report_dimensional_metric(type, name, value, attributes) when type in @valid_types do |
There was a problem hiding this comment.
The main risk I'm noticing in this PR is that report_dimensional_metric collects all the metrics and sends every one up, while report_custom_metric will aggregate each metric during the harvest cycle and only sends each one once.
Which behavior do you want this function to have? I suspect most folks would assume the aggregated behavior. For example, if someone calls this function for every single request, we wouldn't want to copy the name & attributes potentially thousands of times a second..
If you did want that behavior, perhaps send_dimensional_metric is more expressive of what will happen?
There was a problem hiding this comment.
Yeah. I definitely agree that it's not what I would expect.
What are your thoughts on efficiently storing the map of attributes along with the metric name?
Eg I report metric_a with attributes a, b, and c. Then I report metric_a with attributes c, d, and e.
Do I merge the maps? Or does that remain as two metrics?
There was a problem hiding this comment.
I could use something like phash2 on the attributes map and then aggregate matching ones.
There was a problem hiding this comment.
I think that's right, the whole of the attributes makes up a unique metric. The other metric harvester has an optimized aggregation implementation you could steal
There was a problem hiding this comment.
So it would be keyed off of a tuple of the name and the hash of the attributes? The timeslice metric harvester uses the name and the scope, it seems.
tpitale
force-pushed
the
368-dimensional-metrics
branch
from
4f22681
to
8f1b023
Compare
|
Closing in favor of: #408 |
NewRelic.report_dimensional_metric/4