Skip to content

Conversation

@yuvipanda
Copy link
Collaborator

Without this, the hub metrics don't seem to be
scraped.

Without this, the hub metrics don't seem to be
scraped.
@consideRatio
Copy link
Member

Hmmm, I'm quite confident the default behavior of prometheus in the past was to use the first / or only available port of the k8s service in the past.

It would be nice to track down if this has changed or similarly, because if it has there needs to be changes like this in many other charts I figure.

Do you know if this failure relates to a recent update of prometheus? Could this have to do with network policies rather than the port?

@yuvipanda
Copy link
Collaborator Author

I spent a day or so poking around here, and am pretty sure it wasn't networkpolicies. I agree this used to work, but unfortunately it hasn't for a while.

Do you know other installations that are scraping hub metrics? That would be very helpful to check.

@consideRatio
Copy link
Member

consideRatio commented Feb 4, 2021

@yuvipanda okay let's merge this to avoid issues, but in case it becomes relevant to isolate the reason for this in the future can you provide information of...

  • What version of z2jh / prometheus helm chart was used when this fix was required?
  • Was the hub metrics configured to be exposed with or without authentication?

For reference, the neurohackademy deployment was functional without this, and the specs were:

  • JupyterHub version: 0.9.0-n150.h1ce17ed
  • Prometheus version: 11.6.0, appVersion: 2.19.0
  • c.JupyterHub.authenticate_prometheus = False

It seems like the documentation suggest that one is supposed to specify scrape, port, and path annotations and they don't mention anything about default behavior. I think it may make sense to just always be explicit.

Sorry for the slow down!

@consideRatio consideRatio merged commit 56e5ee1 into jupyterhub:master Feb 4, 2021
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Feb 4, 2021
@yuvipanda
Copy link
Collaborator Author

Thanks for the merge, @consideRatio! I agree that being explicit is good anyway, regardless of default behavior.

Unfortunately, this happened a while ago and I've made further changes to the infra since :( I'll report back if I run into stuff like this again, and make sure to note versions.

Thanks!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants