Skip to content

Conversation

@mrodm
Copy link
Contributor

@mrodm mrodm commented Jul 6, 2022

Relates elastic/package-registry#797
Blocked by elastic/package-registry#827 , elastic/package-registry#842

Set the required settings (URL and port) to be able to scrape metrics from the Package Registry.

@mrodm mrodm added the Team:Ecosystem Label for the Packages Ecosystem team label Jul 6, 2022
@mrodm mrodm self-assigned this Jul 6, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 6, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-07-08T15:43:34.896+0000

  • Duration: 28 min 55 sec

Test stats 🧪

Test Results
Failed 0
Passed 770
Skipped 0
Total 770

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 6, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (31/31) 💚
Files 66.379% (77/116) 👍
Classes 61.963% (101/163) 👍
Methods 49.619% (326/657) 👍
Lines 33.07% (2925/8845) 👍
Conditionals 100.0% (0/0) 💚

@mtojek
Copy link
Contributor

mtojek commented Jul 8, 2022

/test

PROFILE: "${PROFILE_NAME}"
healthcheck:
test: ["CMD", "curl", "--cacert", "/etc/ssl/package-registry/ca-cert.pem", "-f", "https://localhost:8080"]
test: ["CMD", "curl", "--cacert", "/etc/ssl/package-registry/ca-cert.pem", "-f", "https://localhost:8080/health"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see there is a /health endpoint available in package-registry:
https://github.com/elastic/package-registry/blob/3fb8bf5ab9b2d4dfc681f5059d21eb4c79f480c5/main.go#L291

It's an empty handler, would it be ok to replace here the healthcheck ? Would it return 200 even when the app is still loading ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, definitely a good idea. I think that you can expand the healthcheck logic to return HTTP 200 if packages.Get returns a positive number of packages.

Copy link
Contributor Author

@mrodm mrodm Jul 8, 2022

Choose a reason for hiding this comment

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

Even if the package-registry (indexer) returns zero packages, it could be a valid scenario, couldn't it ? It would be the case where there are no packages to be served, but the service is running without errors
And if the indexer fails, IIRC the service exits.

Regarding this endpoint, I've performed some quick tests, and this health endpoint fails (as requests to / do) until the service has loaded all the packages:

$  curl -k -v https://localhost:8080/health
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*  CAfile: /etc/ssl/cert.pem
*  CApath: none
* (304) (OUT), TLS handshake, Client hello (1):
* LibreSSL SSL_connect: SSL_ERROR_SYSCALL in connection to localhost:8080
* Closing connection 0
curl: (35) LibreSSL SSL_connect: SSL_ERROR_SYSCALL in connection to localhost:8080

The server starts running after all the packages have been read/validated and loaded. So IIRC if the service is live, then it is also ready, isn't it?

For now, I will revert this change and in this PR I will just set the ports and environment variables needed to enable/use Prometheus metrics.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if the package-registry (indexer) returns zero packages, it could be a valid scenario, couldn't it ? It would be the case where there are no packages to be served, but the service is running without errors
And if the indexer fails, IIRC the service exits.

Well, yes and no. We perform a dry-run validation to make sure that there are any packages, so I would say that we should expect from healthcheck a similar behavior.

and this health endpoint fails (as requests to / do) until the service has loaded all the packages:

I think that the HTTP server starts after loading packages.

@mtojek
Copy link
Contributor

mtojek commented Jul 8, 2022

/test

@mrodm mrodm marked this pull request as ready for review July 8, 2022 16:07
@mrodm mrodm requested review from a team and mtojek July 8, 2022 16:07
@mrodm mrodm merged commit c2311f6 into elastic:main Jul 11, 2022
@mrodm mrodm deleted the expose_prometheus_metrics_epr branch July 11, 2022 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Team:Ecosystem Label for the Packages Ecosystem team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants