Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding Statuscode Handler to Agent Health Extension #1423
base: main
Are you sure you want to change the base?
Adding Statuscode Handler to Agent Health Extension #1423
Changes from 37 commits
2348639
b7fac5e
45536b8
75b024a
b41de09
90a22a3
8c6d037
6ff99f8
667f608
9de9448
1e690e3
4c6adcf
dc129c9
07a06fa
0f76726
965ccdb
a8268fd
8ddb463
3c15e81
947b8f3
a76ef49
abf484b
c849f61
bd33dbc
822680f
d547e1c
5dc40f4
e5bf295
e07463a
a5ba52e
f3c8d46
f64b332
fa4c163
457e48c
2705e87
3083626
9e40bd8
5745ee6
6e0fdd3
8aee0d9
f344518
a7e0415
d91706d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
IMO status code stats should have to be enabled.
agenthealth/metrics
andagenthealth/traces
won't have any allowlisted operations, but are still going to have the handlers created and attached.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 agree with this and I changed it so that we would have to enable statuscodes
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.
nitpick - can be renamed to
is_only_status_code_enabled
also keep it consistent with IsUsageDataEnabled with IsOnlyStatusCodeEnabled
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.
Also any reason to make it a pointer ?
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.
A bool with
omitempty
in the tag is enough.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.
Agreed with mitali. Instead of a pointer, can we treat the default value as false?
Additionally, can you explain a bit more what this field is used for? It seems like a way to set the agent health extension such that it turns the other things off. I think that may be confusing because you could set
IsUsageDataEnabled
to true and also setStatusCodeOnly
to true, so it's not clear what it would actually do (without looking at the implementation).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.
That makes sense. I will rename it. The pointer is used so we can emit it even if it's empty (nil). As for its functionality, I can add a comment, but the main purpose is to ensure that we only use the statuscodehandler when calling the agenthealthextension for a processor, exporter, or receiver, without including other handlers like processstats, clientstats, etc.