-
Notifications
You must be signed in to change notification settings - Fork 81
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 proto-buf definitions for php-fpm metrics #452
Conversation
✅ Deploy Preview for agent-public-docs canceled.
|
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.
LGTM, although I'm still curious if we need three copies of the codebase in this codebase
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #452 +/- ##
==========================================
- Coverage 67.19% 66.85% -0.35%
==========================================
Files 113 113
Lines 12849 12931 +82
==========================================
+ Hits 8634 8645 +11
- Misses 3647 3715 +68
- Partials 568 571 +3
☔ View full report in Codecov by Sentry. |
7986331
to
496ccbd
Compare
// PhpFpm process id | ||
int32 pid = 10 [(gogoproto.jsontag) = "pid"]; | ||
// NGINX agent version | ||
string agent = 11 [(gogoproto.jsontag) = "agent"]; |
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.
not sure why this is required? This is provided elsewhere in the protos https://github.com/nginx/agent/blob/main/sdk/proto/agent.proto#L150
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.
I intend to send over agent version only, similar to how old agent schema has.
// PhpFpm master process total workers/children | ||
int32 workers = 13 [(gogoproto.jsontag) = "workers"]; | ||
// PhpFpm process health | ||
PhpFpmHealth health = 14 [(gogoproto.jsontag) = "health"]; |
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.
should the health be separated from the details?
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.
I thought of sending it out as part of php-information that will have meta details. Do you have any other thoughts on this?
496ccbd
to
93d1dc4
Compare
93d1dc4
to
7eb1e8b
Compare
Proposed changes
Add protobuf definitions for php-fpm metrics along the line of old agent schema
Nginx amplify agent schema for php is currently
Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTING
documentmake install-tools
and have attached any dependency changes to this pull requestREADME.md
)