Require telemetry for API calls #441
Conversation
| $header_value = array( | ||
| 'name' => 'wp-auth0', | ||
| 'version' => WPA0_VERSION, | ||
| 'environment' => array( |
There was a problem hiding this comment.
I'm not sure we support a non-plain object (this environment dictionary). Looking at the previous implementation it seems we do, but may be worth checking if this is easy to query later on the BI app.
joshcanhelp
force-pushed
the
change-telemetry
branch
from
fcc8c8b
to
45c7b1a
Compare
There was a problem hiding this comment.
I've already left you an un-replied comment here https://github.com/auth0/wp-auth0/pull/441/files#r183123221
Anyway, after reviewing the other PRs and seeing how you're using this method I don't like it. Here you return a new array with a key named "Auth0-Client" and the encoded telemetry. The thing is that later, you call this method and access the header by manually typing it's name.
Instead of using it that way, either:
- make this method return the raw telemetry data (and probably rename "get_info_headers" to "generate telemetry") OR
- make this method receive an array or a "request being prepared", and make it be assigned the header inside
An alternative I don't like much for this already implemented scenario is to provide a constant so other methods use that instead of typing the hardcoded name every time they need to access the telemetry header value.
|
@lbalmaceda - Understood and I agree with everything you said. This PR, though, is only to require this data to be sent, not to refactor the entire telemetry system before we know more about it (I have a request in to the data team now). |
joshcanhelp
force-pushed
the
change-telemetry
branch
from
45c7b1a
to
eb264ab
Compare
|
I understand that this PR won't refactor that. However, note that I'm not talking about the contents of the telemetry. I'm pointing out how the method is working today and how it should be changed later. Keeping the method like it is today would require too many wrap-unwrap like operations to just obtain a value. I'll approve this PR, but please track this issue somewhere (new gh issue?). |
|
@lbalmaceda - Understood. This will be addressed once we understand how the telemetry is received and what more we want to (and can) add to it. Good call on tracking, I'll add that as a story. |
Changing API header calls to include telemetry always. Also adding server and mysql information.