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

Percentile filter #73

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

Conversation

podile
Copy link

@podile podile commented Mar 4, 2020

Enhanced code to filter quantiles based on user choice
Included --cql-ssl option to crated encrypted native connection
Added latest version of jars as the older version are having some
vulnerabilities

zegelin and others added 4 commits February 20, 2020 18:12
Enhanced code to filter quantiles based on user choice
Included --cql-ssl option to crated encrypted native connection
Added latest version of jars as the older version are having some
vulnerabilities
Used netty 4.1.42.Final to be inline with another PR
…le-filter

The master branch contains some important fixes.
Copy link

@zegelin zegelin left a comment

Choose a reason for hiding this comment

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

I'm thinking that it would be better to exclude the quantiles in the output writer layer, ie. by just not writing them to the socket, rather than modifying every code path that deals with quantiles.

@zegelin
Copy link

zegelin commented Mar 4, 2020

Check out TextFormatMetricFormatWriter:

We could filter all of them here:

                summary.quantiles.forEach(interval -> {
                    writeMetric(buffer, metricFamily, null, interval.value, summary.labels, interval.quantile.asSummaryLabel());
                });



Iterator<Interval> itr = e.samplingCounting.getIntervals().iterator();
ArrayList<Interval> filtered = Lists.newArrayList();
Copy link

Choose a reason for hiding this comment

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

Creating a new list here isn't necessary. Lets just use Iterables.filter(...).

Copy link
Author

@podile podile Mar 5, 2020

Choose a reason for hiding this comment

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

As you suggested, I will apply filter it in TextFormatMetricFormatWriter and revert the above change.

n660955 added 2 commits March 5, 2020 10:50
Applied quantile filtering while writing metrics into socket.
Corrected formating.
Reverted netty jar version changes.
Appended the new line at the end of files which removed while restoring
previous changes from repo.
@podile podile requested a review from zegelin March 5, 2020 05:30
@podile
Copy link
Author

podile commented Mar 5, 2020

@zegelin thanks for your review and suggestion. I have made the changes, please review.

@podile
Copy link
Author

podile commented Mar 5, 2020

This PR fixes below two issues.

#69
#72

Copy link

@zegelin zegelin left a comment

Choose a reason for hiding this comment

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

Some minor code formatting changes need. Otherwise, looks good to me!

I'll raise a separate improvement ticket to look into some kind of auto linter/formatter so that I don't have to keep bugging you to add a few spaces :)

@podile podile requested a review from zegelin March 6, 2020 04:09
@podile
Copy link
Author

podile commented Mar 6, 2020

@zegelin , thanks for your review. I have fixed formatting issues.

@podile
Copy link
Author

podile commented Mar 13, 2020

@zegelin, can you please approve and merge my pull request if you don't see any issues?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants