-
Notifications
You must be signed in to change notification settings - Fork 5
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 metrics api view implementation #102
Conversation
pom.xml
Outdated
@@ -4,7 +4,7 @@ | |||
|
|||
<groupId>es.tid.fiware</groupId> | |||
<artifactId>keypass</artifactId> | |||
<version>1.2.1</version> | |||
<version>1.3.0</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been a long time since the last Keypass change, so I don't know if we were following the standard versioning procedures here. Are you going to release a new version right now? Maybe this should be labeled as 1.3.0-snapshot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO more over that his PR there is not other change or feature to incorporate to this new release (version 1.3.0)
String id = URLEncoding.decode(policyId); | ||
|
||
Policy p = dao.loadPolicy(tenant, subject, id); | ||
metrics.counter("incomingTransactionRequestSize").inc(tenant.length() + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sum does not really give us the Request Size, don't you think? It gives us the size of the interesting contents (I mean, the values inside the target XML elements) but it leaves away all the XML boilerplate used to codify it. Maybe I'm wrong and that's not the case, but if I'm not, this should be noted in the documentation, as the main reason to have this metrics is bandwidth analysis, and it won't be much useful considering those small bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keypass already uses a internal library for metrics, based on a jetty package. This library is good measuring time, but not total size of requests. This way, using a new coutner, we are rearusing that we could (input data, including headers), that is really near to real request size. The same for responses.
490d44c
to
10d63b6
Compare
Should |
issue #101
curl -i -X GET http://localhost:7070/admin/metrics