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

Additionally send all stats to a single bucket #4758

Merged
merged 1 commit into from Nov 2, 2017

Conversation

Projects
None yet
4 participants
@erasche
Member

erasche commented Oct 5, 2017

Perhaps it is just my inability / missing skills with influxdb, but I can't find another way to aggregate over multiple series names without something like this. Feel free to reject if not interesting.

This allows us to calculate proper 90th percentile graphs for all responses rather than just a few.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Nov 1, 2017

👍 from me - seems like a good idea. I'll ping @dannon explicitly and give him a chance to weigh in and then merge if he doesn't object.

@dannon

This comment has been minimized.

Member

dannon commented Nov 1, 2017

Thanks for the ping. I did look at this, and it doesn't seem right that we'd need to send the message twice here. That said, I did not sit down to figure out how to set up aggregates over series for stats.galaxyproject.org, yet. @erasche You don't think it's possible to do so?

@erasche

This comment has been minimized.

Member

erasche commented Nov 1, 2017

yeah, it's definitely ridiculous that we have to send the message twice, but I haven't found anything that would handle such aggregations / re-sends.

AFAICT influxdb doesn't support this sort of aggregation. E.g. someone asking for a way to do pretty much exactly this and being pointed to this 2+ y/o open issue and that doesn't look encouraging.

I could write some stupid simple statsd proxy that has rules like "if key matches this value, then re-write it with this regex + re-send", but this was a one line change that more or less solved my problem immediately without adding another service I have to maintain.

@dannon

This comment has been minimized.

Member

dannon commented Nov 1, 2017

@erasche Yeah, that's crazy. It just seems like so much of a "not galaxy's problem" that I'm hesitant to add this redundant message. Give me one day to try to figure out a way to do this easily outside galaxy, and then I'll merge this myself until we have a better solution.

@erasche

This comment has been minimized.

Member

erasche commented Nov 2, 2017

Sounds good @dannon! Thanks for the compromise.

@dannon

This comment has been minimized.

Member

dannon commented Nov 2, 2017

@erasche Well, I spent my day doing other things. We can revisit this later :)

@dannon dannon merged commit 15dc193 into galaxyproject:dev Nov 2, 2017

6 checks passed

api test Build finished. 303 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 55 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@erasche

This comment has been minimized.

Member

erasche commented Nov 3, 2017

sounds good! I'll ping this issue if I find anything as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment