Prometheus metrics #1381

kminehart · 2019-04-23T13:15:37Z

Related issue

#1176

Proposed changes

Add custom Prometheus metrics and HTTP middlewares for request count, duration, and size.

Checklist

I have read the contributing guidelines
I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security
vulnerability, I confirm that I got green light (please contact hi@ory.sh) from the maintainers to push the changes.
I signed the Developer's Certificate of Origin
by signing my commit(s). You can amend your signature to the most recent commit by using git commit --amend -s. If you
amend the commit, you might need to force push using git push --force HEAD:<branch>. Please be very careful when using
force push.
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation within the code base (if appropriate)
I have documented my changes in the developer guide (if appropriate)

Further comments

This is still a work in progress. Wanted to make sure that I was heading in the right direction code quality-wise. :)

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

consent/handler.go

consent/metrics.go

metrics/prometheus/metrics.go

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

kminehart · 2019-05-01T20:24:02Z

So this will be a fun PR to merge with all them conflicts. Hopefully we can just force the merge when it's ready since I forgot to sign off on a merge commit.

Highlights:

First class Prometheus support.
- Rather than implement a metrics interface or some kind of abstraction layer for metrics, I propose first class Prometheus support.
  - The Prometheus format is preparing to reach an IETF RFC standard, which means it's a suitable format for most services.
  - The Prometheus format is supported by Sysdig, Datadog, and InfluxDB (As a remote-write target).
- The Prometheus client library is an excellent tool for receiving metrics ad-hoc (in realtime, like number of requests) and for watching for metrics at intervals (things like "# of x in the database), and comes bundled with good HTTP middleware and instrumentation.
The Manager interfaces now also implement the prometheus.Collector.
- Once registered with the Registry (here), the client will continuously retrieve metrics for that manager. This is where SQL queries for "COUNT" and such will happen.
- For metrics not retrieved by Collect, they are incremented as-need / in real-time, and then sent to the Registry in Collect.
A new interface, metrics.Bridge was created. This interface is for pushing metrics to external metric services that may not yet accept the Prometheus format.
The metrics.BridgeManager was created to manage these individual Bridge instances, repeatedly pushing metrics to each external service provided. It is initialized here
The Segment stuff in ory/x/metricsx was moved (mostly) to ory/hydra/metrics as the SegmentBridge

Todo:

Include path in HTTP metrics middleware (use ory/x/metricsx code)
Implement the SQL queries for metrics in the managers
Clean up & comment code. Add package-level comments and comments for exported functions.

aeneasr · 2019-05-02T13:35:04Z

Nice! What should I look out for when reviewing the WIP changes?

Regarding Segment, it is very important that the existing fields, keys, and events stay unchanged. I also want to avoid to move stuff from x/metrics back here as x/metrics is standardized across all repos and we do not want to diverge per project.

kminehart · 2019-05-02T14:36:35Z

Gotya. That is actually the kind of information I was looking for! I'm looking for general ideas more than anything, like maybe, "this stuff would be better in ory/x", or "this isn't suitable for the manager" kind of stuff.

I'll make sure that the fields, keys, and events stay the same.

There's not a whole lot to review; usually when I open a WIP PR I'm just making sure I'm heading in the right direction.

aeneasr · 2019-05-03T12:59:42Z

Ok, perfect!

Would it make sense to have the bridge in x?

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

kminehart · 2019-05-05T23:50:28Z

Would it make sense to have the bridge in x?

I was actually thinking the same thing!

I went ahead and did that.

In order for this to pass, it'll need this: ory/x#49

There might be some metrics missing or collected in an inopportune time, or just flat-out wrong, as my understanding of Hydra's code doesn't expand far beyond changes I've made.

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

aeneasr

Interim review submission to see if my comments got lost or not :/

aeneasr · 2019-05-07T13:05:59Z

client/metrics.go

+
+var (
+	metricClients = prometheus.NewGauge(prometheus.GaugeOpts{
+		Namespace: "hydra",


Would it make sense to have the namespace fixed as a const somewhere? We should also decide if it's ory-hydra, ory_hydra, ORY Hydra, hydra or any other combination.

aeneasr · 2019-05-07T13:06:42Z

cmd/server/handler.go

@@ -77,6 +82,7 @@ func RunServeAdmin(version, build, date string) func(cmd *cobra.Command, args []
 	return func(cmd *cobra.Command, args []string) {
 		d := driver.NewDefaultDriver(
 			logrusx.New(),
+			prometheus.NewRegistry(),


If this is always prometheus.NewRegistry(), wouldn't it make more sense to create that within the registry_base?

aeneasr · 2019-05-07T13:19:51Z

cmd/server/handler.go

+		ctx     = context.Background()
+	)
+
+	optOut, err := cmd.Flags().GetBool("sqa-opt-out")


Do we need all of this here or could we hide that in metricsx as it was previously?

aeneasr · 2019-05-07T13:20:58Z

cmd/server/handler.go

-		},
-	)
+		}
+		b, err := metricsx.NewFormattedSegmentBridge(ctx, s, l, d.Registry().PrometheusManager().Registry)


Ok I see - so segment actually works (mostly) independent of Prometheus except for e.g. memory stats?

aeneasr · 2019-05-07T13:22:04Z

client/manager_sql.go

@@ -351,9 +352,56 @@ func (m *SQLManager) GetClients(ctx context.Context, limit, offset int) (clients
 func (m *SQLManager) CountClients(ctx context.Context) (int, error) {
 	var n int
 	if err := m.DB.QueryRow("SELECT count(*) FROM hydra_client").Scan(&n); err != nil {
-		fmt.Println(err.Error())


aeneasr · 2019-05-07T13:25:35Z

consent/handler.go

+
+	defer func(p *HandledLoginRequest, err error) {
+		recordAcceptLoginRequest(p, err)
+	}(&p, err)


Relying on a global error is very flaky - there are several places where err will be overshadowed, e.g. here and here and here. I'm not sure about other projects but I feel that multiple return values usually re-define/overshadow the original err scope:

package main import ( "fmt" ) func main() { a := "a" fmt.Println(a) // "a" b, a := "b", "c" fmt.Println(a) // "c" fmt.Println(b) // "b" }

aeneasr · 2019-05-07T13:29:19Z

consent/manager_memory.go

@@ -118,13 +119,17 @@ func (m *MemoryManager) RevokeSubjectClientConsentSession(ctx context.Context, u
 				// do nothing
 			} else if err != nil {
 				return err
+			} else {


This else is not really required right?

kminehart · 2019-05-11T17:03:56Z

I'm gonna close this and re-open a new one when its ready.

kminehart added 6 commits April 18, 2019 11:17

Start adding prometheus metrics

85fc976

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

Merge remote-tracking branch 'upstream/master' into prometheus-metrics

c7ecb4d

update the driver

051b4f7

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

revert registry changes

af6defd

Add more prometheus metrics

eeac3a2

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

Remove unused / bad functions from metrics/prometheus

5f21dbf

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

kminehart changed the title ~~Prometheus metrics~~ WIP: Prometheus metrics Apr 23, 2019

Merge remote-tracking branch 'upstream/master' into prometheus-metrics

b0b492c

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

aeneasr reviewed Apr 23, 2019

View reviewed changes

kminehart added 6 commits April 23, 2019 22:13

Instrument prometheus metrics with maintained Registry

e395540

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

Remove log.println, set HandledConsentRequest.ConsentReqest

1752e4e

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

Add some additional metrics for consent package

27e481f

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

make format

a902f86

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

Fix cardinality of reject metrics

9c1aee8

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

First-class support of Prometheus

03e5af0

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

kminehart added 4 commits May 3, 2019 14:54

prepare for merge

4979c2b

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

Move hydra/metrics content to ory/x/metricsx

c10574e

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

revert writekey

530a5df

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

Finish up prometheus metrics

ad0b8e8

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

kminehart changed the title ~~WIP: Prometheus metrics~~ Prometheus metrics May 5, 2019

padding stats w/ comments

54563b0

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>

aeneasr reviewed May 7, 2019

View reviewed changes

kminehart closed this May 11, 2019

This was referenced Jan 18, 2022

[Snyk] Security upgrade node-fetch from 2.6.0 to 3.1.1 piotrkpc/hydra#8

Open

[Snyk] Security upgrade node-fetch from 2.6.1 to 3.1.1 piotrkpc/hydra#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prometheus metrics #1381

Prometheus metrics #1381

kminehart commented Apr 23, 2019

kminehart commented May 1, 2019 •

edited

Loading

aeneasr commented May 2, 2019

kminehart commented May 2, 2019 •

edited

Loading

aeneasr commented May 3, 2019

kminehart commented May 5, 2019 •

edited

Loading

aeneasr left a comment

aeneasr May 7, 2019

aeneasr May 7, 2019

aeneasr May 7, 2019

aeneasr May 7, 2019

aeneasr May 7, 2019

aeneasr May 7, 2019

aeneasr May 7, 2019

kminehart commented May 11, 2019

Prometheus metrics #1381

Prometheus metrics #1381

Conversation

kminehart commented Apr 23, 2019

Related issue

Proposed changes

Checklist

Further comments

kminehart commented May 1, 2019 • edited Loading

aeneasr commented May 2, 2019

kminehart commented May 2, 2019 • edited Loading

aeneasr commented May 3, 2019

kminehart commented May 5, 2019 • edited Loading

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr May 7, 2019

Choose a reason for hiding this comment

aeneasr May 7, 2019

Choose a reason for hiding this comment

aeneasr May 7, 2019

Choose a reason for hiding this comment

aeneasr May 7, 2019

Choose a reason for hiding this comment

aeneasr May 7, 2019

Choose a reason for hiding this comment

aeneasr May 7, 2019

Choose a reason for hiding this comment

aeneasr May 7, 2019

Choose a reason for hiding this comment

kminehart commented May 11, 2019

kminehart commented May 1, 2019 •

edited

Loading

kminehart commented May 2, 2019 •

edited

Loading

kminehart commented May 5, 2019 •

edited

Loading