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 gzip support #223

Merged
merged 6 commits into from
Aug 2, 2024
Merged

Add gzip support #223

merged 6 commits into from
Aug 2, 2024

Conversation

Athishpranav2003
Copy link
Contributor

Added support for gzip compression while exposing metrics.
Tested locally with prometheus server
image

#219

Lusitaniae
Lusitaniae reviewed Jul 29, 2024
View reviewed changes
README.md Outdated
@@ -63,6 +63,7 @@ More configuration parameters:
- `port`: listen port (default: 24231)
- `metrics_path`: metrics HTTP endpoint (default: /metrics)
- `aggregated_metrics_path`: metrics HTTP endpoint (default: /aggregated_metrics)
- `content_encoding`: encoding format for the exposed metrics (default: identity)
Copy link

@Lusitaniae Lusitaniae Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

usually you'd read Accept-Encoding header from the request to determine how to encode the response

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is true
But in our case Prometheus server accepts both gzip and identity (it's not specific) so the request header has both in accept encoding. We cannot determine whether we only need compressed or identity. That's why I configured it in server side

@Lusitaniae
Copy link

amazing turnaround, much appreciated for your efforts @Athishpranav2003!

@Athishpranav2003
Copy link
Contributor Author

Thanks @Lusitaniae
One help
The signoff check is not passing for me from first.
Initially I had dummy author creds in local git so it messed it. I reverted to original stuff and still not passing the check. Can you help me with that alone.
Also who has write access to merge this?

@Lusitaniae
Copy link

Looks like you just need to follow these steps:

In your local branch, run: git rebase HEAD~2 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin add-gzip-support

https://github.com/fluent/fluent-plugin-prometheus/pull/223/checks?check_run_id=28060535686

@Athishpranav2003
Copy link
Contributor Author

@ashie can you please check this

@ashie
Copy link
Member

ashie commented Jul 30, 2024

@Athishpranav2003 Thanks for your contribution!
Could you follow DCO as @Lusitaniae mentioned?
We require Signed-off-by: field in each commit message.

Looks like you just need to follow these steps:

In your local branch, run: git rebase HEAD~2 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin add-gzip-support

https://github.com/fluent/fluent-plugin-prometheus/pull/223/checks?check_run_id=28060535686

@Athishpranav2003
Added encoding support for exposed metrics
0a62415 
Signed-off-by: Athishpranav2003 <[email protected]>
@Athishpranav2003
Updated readme
87189d9 
Signed-off-by: Athishpranav2003 <[email protected]>
@Athishpranav2003
Copy link
Contributor Author

@ashie I have made the changes. In addition i have added zlib as dependency to avoid break incase some system misses it(highly unlikely).

@ashie
Copy link
Member

ashie commented Jul 30, 2024

I'll fix CI for Ruby 2.7 in another pull request, so please leave it.

@Athishpranav2003 Athishpranav2003 mentioned this pull request Jul 31, 2024
Closed
@Athishpranav2003
Copy link
Contributor Author

@ashie do you need help in testing this locally before merging

@ashie
Copy link
Member

ashie commented Jul 31, 2024

It looks good, I'll merge this soon after checking if we missed anything in relation to the surrounding code.

ashie
ashie requested changes Aug 1, 2024
View reviewed changes
Copy link
Member

@ashie ashie left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have made some comments about some things I would like to see fixed.
In addition, it would be better to add unit tests for this feature if possible.

lib/fluent/plugin/in_prometheus.rb Outdated Show resolved Hide resolved
lib/fluent/plugin/in_prometheus.rb Outdated Show resolved Hide resolved
lib/fluent/plugin/in_prometheus.rb Outdated Show resolved Hide resolved
lib/fluent/plugin/in_prometheus.rb Outdated Show resolved Hide resolved
lib/fluent/plugin/in_prometheus.rb Outdated Show resolved Hide resolved
lib/fluent/plugin/in_prometheus.rb Outdated Show resolved Hide resolved
lib/fluent/plugin/in_prometheus.rb Outdated Show resolved Hide resolved
@Athishpranav2003
Copy link
Contributor Author

Ok
I will take a look at the comments.

Regarding UTs there is no UTs written for the plugin.
So the UTs have go in a seperate PR if we want a Minitest kinda UTs. For now I was manually testing the changes.

@Athishpranav2003
Addressed comments
29d0dca 
Signed-off-by: Athishpranav2003 <[email protected]>
@Athishpranav2003
Copy link
Contributor Author

@ashie i have addressed your comments and also refactored some small part of my code

@Athishpranav2003 Athishpranav2003 requested a review from ashie August 1, 2024 12:45
@Athishpranav2003
Added UTs
08389fd 
Signed-off-by: Athishpranav2003 <[email protected]>
@Athishpranav2003
Copy link
Contributor Author

@ashie i have added UTs now
please take a look at it when u are free

ashie
ashie requested changes Aug 2, 2024
View reviewed changes
lib/fluent/plugin/in_prometheus.rb Outdated Show resolved Hide resolved
spec/fluent/plugin/in_prometheus_spec.rb Show resolved Hide resolved
spec/fluent/plugin/in_prometheus_spec.rb Outdated Show resolved Hide resolved
@Athishpranav2003
Addressed comments
d1a545b 
Signed-off-by: Athishpranav2003 <[email protected]>
@Athishpranav2003
Copy link
Contributor Author

@ashie addressed the comments

@Athishpranav2003 Athishpranav2003 requested a review from ashie August 2, 2024 04:45
@Athishpranav2003 @ashie
Removed dep
72e2483 
Signed-off-by: Athishpranav2003 <[email protected]>
ashie
ashie approved these changes Aug 2, 2024
View reviewed changes
Copy link
Member

@ashie ashie left a comment

Choose a reason for hiding this comment

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

LGTM

@ashie
Copy link
Member

ashie commented Aug 2, 2024

I've removed wrongly added fluent-plugin-prometheus-2.1.0.gem in your commit.

@ashie ashie merged commit 11cf2c2 into fluent:master Aug 2, 2024
4 of 5 checks passed
@ashie
Copy link
Member

ashie commented Aug 2, 2024

Thanks!

@ashie ashie mentioned this pull request Aug 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashie ashie ashie approved these changes

@Lusitaniae Lusitaniae Awaiting requested review from Lusitaniae

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Athishpranav2003 @Lusitaniae @ashie