Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add max and min functions for mal down sampling #11778

Merged
merged 9 commits into from
Jan 16, 2024

Conversation

peachisai
Copy link
Contributor

@peachisai peachisai commented Jan 15, 2024

  • Tests(including UT, IT, E2E) are added to verify the new feature.
  • Update the CHANGES log.

User Case

metricsRules:
  - name: consumer_latency
    exp: rocketmq_group_get_latency_by_storetime.sum(['cluster','topic','group']).downsampling(MAX)

The opentelemetry metrics

rocketmq_consumer_message_size{cluster="DefaultCluster",broker="broker-a",topic="topic1",group="group2",} 21350.0
rocketmq_consumer_message_size{cluster="DefaultCluster",broker="broker-b",topic="topic1",group="group2",} 7700.0

The sum aggregate method use 'cluster','topic' and 'group' as aggregate-by params,so I can sum the metrics of the two brokers, and then get the max message size instead of the default avg function value

The same scenario for min function

peachisai and others added 7 commits January 15, 2024 09:14
…of github.com:peachisai/skywalking into Support-'max'-and-min-functions-for-mal-down-sampling

# Conflicts:
#	docs/en/changes/changes.md
…into uat2

# Conflicts:
#	docs/en/changes/changes.md
Support-'max'-and-min-functions-for-mal-down-sampling
@wu-sheng
Copy link
Member

Could you provide the use cases?

@wu-sheng wu-sheng added TBD To be decided later, need more discussion or input. backend OAP backend related. feature New feature labels Jan 15, 2024
@wu-sheng wu-sheng requested review from mrproliu and wu-sheng January 15, 2024 12:40
@peachisai
Copy link
Contributor Author

Could you provide the use cases?

I updated the use case

@wu-sheng
Copy link
Member

wu-sheng commented Jan 15, 2024

Could you provide the use cases?

I updated the use case

Why do you need a max value? I think in most case, percentile should be better to measure latency? Does max bring a special value?

@peachisai
Copy link
Contributor Author

Could you provide the use cases?

I updated the use case

Why do you need a max value? I think in most case, percentile should be better to measure latency? Does max bring a special value?

I prefer the maximum value. This looks more intuitive. For example, with size and offset metrics.

@wu-sheng
Copy link
Member

Size and offset make sense. But about latency, percentile is better.

@wu-sheng wu-sheng removed the TBD To be decided later, need more discussion or input. label Jan 15, 2024
@wu-sheng wu-sheng added this to the 10.0.0 milestone Jan 15, 2024
@wu-sheng
Copy link
Member

Please update the description for the meaningful use cases.

}
}

@Entrance
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Entrance is misused here, and this error exists in many other MAL functions. This annotation is used in MAL engine use, in MAL, accept method is the main entrance by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to fix existing codes.

@peachisai
Copy link
Contributor Author

Please update the description for the meaningful use cases.

Updated

@wu-sheng wu-sheng merged commit af4f469 into apache:master Jan 16, 2024
167 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related. feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants