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.

Sign up for GitHub

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

Jump to bottom

Add arch in wave metrics #791

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add arch in wave metrics #791

wants to merge 12 commits into from

Conversation

munishchouhan
Copy link
Member

This PR will add arch in wave metrics

@munishchouhan
Added arch in metrics
b0c8bfb 
Signed-off-by: munishchouhan <[email protected]>
@munishchouhan munishchouhan linked an issue Jan 31, 2025 that may be closed by this pull request
Capture wave metrics by container architecture #789
Open
3 tasks
@munishchouhan munishchouhan self-assigned this Jan 31, 2025
@munishchouhan munishchouhan marked this pull request as draft January 31, 2025 16:01
@pditommaso
Copy link
Collaborator

@munishchouhan what's missing to have this ready to review?

@munishchouhan
Copy link
Member Author

@munishchouhan what's missing to have this ready to review?

unit test and testing

@pditommaso
Copy link
Collaborator

Fair enough

@pditommaso
Copy link
Collaborator

This is escalating in something much bigger than expected. Is it really needed a new set of endpoints?

pditommaso
pditommaso reviewed Feb 5, 2025
View reviewed changes
src/main/groovy/io/seqera/wave/filter/MetricsQueryParamValidationFilter.groovy
@CompileStatic
@Filter(["/v1alpha2/metrics/**", "/v1alpha3/metrics/**"])
@Requires(property = 'wave.metrics.enabled', value = 'true')
class MetricsQueryParamValidationFilter implements HttpServerFilter {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why a filter for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

this validation function is common for all the endpoints in this controller, so instead of calling for every endpoint
I created this filter to reduce boilerplate code

@munishchouhan
Copy link
Member Author

This is escalating in something much bigger than expected. Does it really need a new set of endpoints?

currently, these APIs are being called periodically in product analytics
So new APIs are created to not break those scripts and to make the change gracefully.
First, deprecate them, and once the analytics scripts are changed and tested, then remove them.

@munishchouhan
Copy link
Member Author

tested locally:
Screenshot 2025-02-05 at 15 55 08
Screenshot 2025-02-05 at 15 54 59
Screenshot 2025-02-05 at 15 54 43
Screenshot 2025-02-05 at 15 54 27
Screenshot 2025-02-05 at 15 54 14
Screenshot 2025-02-05 at 15 54 05
Screenshot 2025-02-05 at 15 46 42
Screenshot 2025-02-05 at 15 46 04
Screenshot 2025-02-05 at 15 45 26
Screenshot 2025-02-05 at 15 44 46
Screenshot 2025-02-05 at 15 42 51
Screenshot 2025-02-05 at 15 42 30
Screenshot 2025-02-05 at 15 41 54

@munishchouhan munishchouhan marked this pull request as ready for review February 5, 2025 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pditommaso pditommaso pditommaso left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@munishchouhan munishchouhan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Capture wave metrics by container architecture
2 participants
@munishchouhan @pditommaso