Skip to content
This repository was archived by the owner on Jan 8, 2023. It is now read-only.

feat: add metrics for request length and response length#48

Merged
joao-fontenele merged 4 commits into
joao-fontenele:masterfrom
matsumana:feature/add-request-response-legth-metrics
Apr 24, 2021
Merged

feat: add metrics for request length and response length#48
joao-fontenele merged 4 commits into
joao-fontenele:masterfrom
matsumana:feature/add-request-response-legth-metrics

Conversation

@matsumana

Copy link
Copy Markdown
Contributor

Hi,
I would like to add the new metrics for Request length and Response length.

Motivation:
If an API handles a large JSON in request or response, the latency might get worse.
So I would like to monitor them.

@matsumana

Copy link
Copy Markdown
Contributor Author

@joao-fontenele Could you take a look this PR?

@joao-fontenele joao-fontenele left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hello @matsumana

These are nice to have metrics, The only problem I see with this is making collecting it mandatory. If you make these changes optional, It would be good to merge

@matsumana

Copy link
Copy Markdown
Contributor Author

Thank you for reviewing.
I have updated.

@matsumana

Copy link
Copy Markdown
Contributor Author

@joao-fontenele Could you take a look this again?

@matsumana

Copy link
Copy Markdown
Contributor Author

@joao-fontenele I am sorry to bother you. Could you take a look this again?

@joao-fontenele-daitan joao-fontenele-daitan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@joao-fontenele

Copy link
Copy Markdown
Owner

@matsumana there's a conflict with master, if you resolve the conflicts I can merge it

@matsumana

Copy link
Copy Markdown
Contributor Author

@joao-fontenele I could not find any conflicts.
According to the following message, seems there is no conflicts.

This branch has no conflicts with the base branch

@joao-fontenele

joao-fontenele commented Apr 24, 2021

Copy link
Copy Markdown
Owner

yep, my bad I was selecting the option to rebase and merge, and was seeing a conflict, but it's actually updated

@joao-fontenele joao-fontenele merged commit 30ec2ed into joao-fontenele:master Apr 24, 2021
@matsumana matsumana deleted the feature/add-request-response-legth-metrics branch April 24, 2021 01:34
@matsumana

Copy link
Copy Markdown
Contributor Author

Thank you for reviewing.
Since I would like to use this feature, I would appreciate if you could release the new version.

@joao-fontenele

Copy link
Copy Markdown
Owner

@matsumana sorry for the delay, man, and thanks for the contribution.

Already released the new version

@matsumana

Copy link
Copy Markdown
Contributor Author

Thank you so much!

@matsumana matsumana restored the feature/add-request-response-legth-metrics branch April 24, 2021 05:10
@matsumana matsumana deleted the feature/add-request-response-legth-metrics branch April 24, 2021 05:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants