Isolate MAL CounterWindow cache by metric name.

[weixiang1862]

Fix MAL CounterWindow Isolation.

• Add a unit test to verify that the fix works.
• Explain briefly why the bug exists and how to fix it.

• If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes MAL CounterWindow may should use `metric name` as ID name instead of `Sample name`. #11522.
• Update the CHANGES log.

[weixiang1862]

Test not ready, I want confirm code first.

[wu-sheng]

Please make sure the codes work first. Otherwise, we don't know it is missing change or on purpose.

[weixiang1862]

Please make sure the codes work first. Otherwise, we don't know it is missing change or on purpose.

Sorry, I will complete test first.

[weixiang1862]

Unit Test is ready.

[wu-sheng]

So, you are forwarding the metric cache into this runtime CACHE for MAL runtime? Is this your basic principle?

[weixiang1862]

Yes, ThreadLocal has sampleFamilies before, I newly add metric name.

[hanahmily]

The metrics name is from the YAML, which is constant instead of a variable value. It's better to add it to the "ExpressionDelegate" as a field. Then inject the metric name into the samplefamilily in "propertyMissing"

[weixiang1862]

@hanahmily Ok, thanks. I will try this way.

[wu-sheng]

Let's wait for others to review this. I am not familiar with this part of the codes.

[weixiang1862]

Let's wait for others to review this. I am not familiar with this part of the codes.

Ok.

[wu-sheng]

One question, if two expressions share the same metric source(counter), but use different rate, how does the cache keep the longer time windows for the further calculation?

[weixiang1862]

One question, if two expressions share the same metric source(counter), but use different rate, how does the cache keep the longer time windows for the further calculation?

Two expression will create two cache in window, one for shorter and one for longer.

[weixiang1862]

One question, if two expressions share the same metric source(counter), but use different rate, how does the cache keep the longer time windows for the further calculation?

Two expression will create two cache in window, one for shorter and one for longer.

Seems use metric name as id name will cause memory waste, expressions can not share cache in some cases？

[wu-sheng]

One question, if two expressions share the same metric source(counter), but use different rate, how does the cache keep the longer time windows for the further calculation?

Two expression will create two cache in window, one for shorter and one for longer.

Seems use metric name as id name will cause memory waste, expressions can not share cache in some cases？

Yes, but it seems easier at the implementation level. Do you have idea to optimize it by using the longer range?

[weixiang1862]

Seems use metric name as id name will cause memory waste, expressions can not share cache in some cases？

Yes, but it seems easier at the implementation level. Do you have idea to optimize it by using the longer range?

I have no idea about optimize yet.

[wu-sheng]

OK, let's keep the status for now. I think this not a big issue of a little more extra memory cost.

[weixiang1862]

Seems use metric name as id name will cause memory waste, expressions can not share cache in some cases？

Yes, but it seems easier at the implementation level. Do you have idea to optimize it by using the longer range?

How about use expression prefix as Id name and maintain a field in window to record lower bound calculate by long rate,
then metrics can share same cache if they has same expression prefix and window just need keep the longer one cache?

For example:

- name: http_requests_pt1m
  exp: nginx_http_requests_total.sum(['service']).rate('PT1M')
- name: http_requests_pt1h
  exp: nginx_http_requests_total.sum(['service']).rate('PT1H')
- name: http_4xx_requests
  exp: nginx_http_requests_total.tagMatch("status", "400|401|403|404|405").sum(['service']).rate('PT1M')

the 3 metrics above will create window by 2 ids, and the first one window length is 1h, the second one window length is 1M:

ID(name=nginx_http_requests_total, labels={service=nginx::portal})
ID(name=nginx_http_requests_total.tagMatch("status", "400|401|403|404|405"), labels={service=nginx::portal})

[wu-sheng]

Let's not bring this complexity into the codes this time. Let's keep one expression per cache if it is a counter.

[weixiang1862]

Let's not bring this complexity into the codes this time. Let's keep one expression per cache if it is a counter.

Ok, I will follow this.

[weixiang1862]

The metrics name is from the YAML, which is constant instead of a variable value. It's better to add it to the "ExpressionDelegate" as a field. Then inject the metric name into the samplefamilily in "propertyMissing"

I have add a field named metricName in ExpressionDelegate and inject it to samplefamily runningcontext in "propertyMissing"

[wu-sheng]

@kezhenxu94 @wankai123 Could any of you run another check?

[weixiang1862]

Thanks.