Skip to content

Conversation

@Chengxuan
Copy link
Contributor

@Chengxuan Chengxuan commented Feb 20, 2025

Proposed changes

As elaborated in hyperledger/firefly-common#167, there is a need to expose more endpoints for operational checks.

This PR propose a new monitoring sections in the configuration file for configurations of the monitoring server:

  • whether to enable the monitoring server
  • a metricsPath

The new monitoring server itself is a replacement for the existing metrics server.

  • Expose a liveness check route
  • Expose the metrics routes

As a result the existing metrics section is marked as deprecated.

Adding monitoring routes for the latest best practices. The existing metrics config is marked as deprecated.

The FireFly API path has been split into api and spi and then namespaced api. So adopting the common utils will need some dedicated time, which I didn't manage to do.


Types of changes

  • Bug fix
  • New feature added
  • Documentation Update

Please make sure to follow these points

  • I have read the contributing guidelines.
  • I have performed a self-review of my own code or work.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generates no new warnings.
  • [?] My Pull Request title is in format < issue name > eg Added links in the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • My changes have sufficient code coverage (unit, integration, e2e tests).

Screenshots (If Applicable)


Other Information

Any message for the reviewer or kick off the discussion by explaining why you considered this particular solution, any alternatives etc.

@Chengxuan Chengxuan requested a review from a team as a code owner February 20, 2025 20:30
@Chengxuan Chengxuan force-pushed the monitoring-routes branch 2 times, most recently from f079a84 to 919495d Compare February 21, 2025 06:32
@codecov
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.95%. Comparing base (611834d) to head (2aa4db2).
Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1640   +/-   ##
=======================================
  Coverage   99.95%   99.95%           
=======================================
  Files         337      337           
  Lines       29504    29524   +20     
=======================================
+ Hits        29492    29512   +20     
  Misses          8        8           
  Partials        4        4           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

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

Looks good @Chengxuan - We will need to add this to the release notes for folks to be aware of

Comment on lines +484 to +500
go func() {
if err := server.ListenAndServeTLS(publicKeyFile.Name(), privateKeyFile.Name()); err != nil && err != http.ErrServerClosed {
log.Fatalf("ListenAndServeTLS(): %v", err)
}
}()

// Wait for the server to be ready
for {
conn, err := tls.Dial("tcp", server.Addr, &tls.Config{
InsecureSkipVerify: true,
})
if err == nil {
conn.Close()
break
}
time.Sleep(10 * time.Millisecond)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for this Cheng!

@EnriqueL8 EnriqueL8 changed the title Monitoring routes Rename metrics to monitoring to support additional routes Feb 24, 2025
@EnriqueL8 EnriqueL8 merged commit d484497 into hyperledger:main Feb 25, 2025
19 checks passed
@EnriqueL8 EnriqueL8 deleted the monitoring-routes branch February 25, 2025 09:25
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