feat: Add Metrics Trait #480
Conversation
yash30201
commented
Sep 14, 2023
yash30201
commented
- Add Metrics Trait
- Add unit tests for metrics trait
* Add Metrics Trait * Add unit tests for metrics trait
yash30201
added
type: feature request
There was a problem hiding this comment.
Great PR! I've left a few initial thoughts, and here are a few more:
Does this need to be in a trait? Traits are only necessary if multiple classes share this functionality. If it's exposed as a trait, but not actually used by classes IN the auth library, it might be better to put it in the library where the classes are which DOES use it (e.g. GAX).
Is there another PR coming which shows how this trait will be implemented? It's hard to review this PR without that one.
| public static $credTypeSaAssertion = 'cred-type/sa'; | ||
| public static $credTypeSaJwt = 'cred-type/jwt'; | ||
| public static $credTypeSaMds = 'cred-type/mds'; | ||
| public static $credTypeSaImpersonate = 'cred-type/imp'; |
There was a problem hiding this comment.
At least based on their usage in this PR, these should all be private by default. But I think I'd get rid of them all together and just replace their usage with the strings themselves - they're only used in one place each, and they're not variable, so they shouldn't be variables.
They could be constants, but as traits don't allow constants, it won't work here.
|
|
||
| // TODO: Find a way to get the auth version | ||
| // Auth library version | ||
| public static $version = '10.0.0'; |
There was a problem hiding this comment.
I'd suggest making this a placeholder function instead, like this:
private static getAuthLiberaryVersion(): string
{
throw new LogicException('This must be implemented by the class which uses the trait');
}Also, why is the placeholder 10.0.0?
| /** | ||
| * Returns header string for metadata server ping request. | ||
| */ | ||
| public function getMdsPingHeader() |
There was a problem hiding this comment.
It's hard to say without seeing where these methods will be implemented, but I am fairly confident these methods can all be private, as they'll be used internally to whatever class is making these calls.
|
Will work on a different approach as per design doc. |
