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

Add CpuTime and Duration quantiles to worker metrics #43

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

m0ar
Copy link
Contributor

@m0ar m0ar commented Oct 13, 2021

This PR extends the worker metrics by providing quantiles for used CPU time and duration for invocations:

2021-10-13-135301_659x275_scrot

I also took the liberty of resolving the errors for trying to fetch non-free metrics on free zones:

zone 'xxx' does not have access to the path

This is done by adding a free_tier variable/flag which disables non-free metric polling. It's backwards compatible, if not explicitly set the default behaviour is to scrape everything. Currently, this leaves only worker metrics. In the future, one can imagine allowing to configure scraping of httpRequestGroups1h (instead of 1m) for free tier metrics but didn't want to scope creep too much. :)

@@ -4,7 +4,7 @@ import (
"context"
"time"

cloudflare "github.com/cloudflare/cloudflare-go"
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 alias was unused before

@@ -7,7 +7,7 @@ import (
"sync"
"time"

cloudflare "github.com/cloudflare/cloudflare-go"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here

@m0ar m0ar mentioned this pull request Oct 14, 2021
@m0ar
Copy link
Contributor Author

m0ar commented Oct 15, 2021

@martinhaus Do you want me to add any janitorial changes to this PR? Updating versions somewhere and whatnot :)

@martinhaus martinhaus self-requested a review October 15, 2021 09:14
@martinhaus
Copy link
Member

LGTM, thanks @m0ar! I'll make a release and update the versions in Heml :)

@martinhaus martinhaus merged commit 96e6d4d into lablabs:master Oct 15, 2021
@m0ar
Copy link
Contributor Author

m0ar commented Oct 15, 2021

@martinhaus Sweet, thanks yourself. :)

There is one thing I'm still trying to figure out, and that is how the heck to set up the API token. Whenever I try using this service without the global API key, I get nothing...

@martinhaus
Copy link
Member

@m0ar You should be able to create a token here: https://dash.cloudflare.com/profile/api-tokens
I use the Read analytics and logs template and it works fine.

@m0ar
Copy link
Contributor Author

m0ar commented Oct 15, 2021

@martinhaus Yeah, I've set that up. The issue is that we only pay for worker subscriptions, meaning our zone is on the free tier. This is why I added the free-tier flag, to squelch our errors. However, it seems that the fetchAccounts function from #21 yields an empty slice for me (which fails the worker metrics silently), so there is some other permission that seems to be missing... I'll send another PR when I figure it out :)

@martinhaus
Copy link
Member

@m0ar perfect, PR would be very welcomed :)

@m0ar
Copy link
Contributor Author

m0ar commented Oct 15, 2021

Hmm, the hickup seems to be that api.Accounts working as "returns all accounts the logged in user has access to", which doesn't really make sense without X-AUTH-EMAIL identifying the user. I'm not able to query this endpoint with curl either using the bearer token. :/

@m0ar
Copy link
Contributor Author

m0ar commented Oct 15, 2021

I think this might be an upstream issue related to auth tokens.. I've posted a bug report with cloudflare-go: cloudflare/cloudflare-go#725

@m0ar
Copy link
Contributor Author

m0ar commented Oct 18, 2021

@martinhaus Found the issue! Account Settings:Read is required on the token for the account listing to work. :)

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.

None yet

2 participants