Skip to content
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

Added Get-GSChromeDeviceTelemetry #365

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Foggy2
Copy link
Contributor

@Foggy2 Foggy2 commented Mar 1, 2022

Added Get-GSChromeDeviceTelemetry
Added ChromeMamnagement API to NuGetDependencies.json
Updated Newtonsoft.Json version in NuGetDependencies.json to support the ChromeManagement API

This will allow use of the customers.telemetry.devices method of the Chrome Management API.

Haven't done anything on this level before so let me know of any feedback.

Added Get-GSChromeDeviceTelemetry
Added ChromeMamnagement API to NuGetDependencies.json
Updated Newtonsoft.Json version in NuGetDependencies.json to support the ChromeManagement API
@FISHMANPET
Copy link
Collaborator

Hey @Foggy2 this looks really good. I assume you've been able to test this yourself in your Org? I don't have access to Chrome devices in mine so I can't test it myself.

@FISHMANPET FISHMANPET added this to the v2.37.0 milestone Apr 1, 2022
@Foggy2
Copy link
Contributor Author

Foggy2 commented Apr 3, 2022

Hey @FISHMANPET. Yes, I have been using this for the last couple weeks without issue.

In the interests of consistency I would like your thoughts on the naming? I was unsure if I should call it Get-GSChromeDeviceTelemetry or Get-GSChromeTelemetry. Ultimately I ended up with specifying Device in the name as this one appears to relate only to Chrome hardware devices. However, the Chrome Management API where this request is serviced from does include methods that cover both Chrome browser and Chrome devices at the same time.

We also already have a function `Get-GSChromeOSDevice' as an example too which is another variation on the naming convention.

@FISHMANPET
Copy link
Collaborator

Naming can be tricky, especially with some of Google's APIs, but my opinion is that you've made the right choice here. While this is in the "Chrome" API, you're right this particular endpoint only applies to devices, so I think it makes sense to include it in the name.

The other function uses the admin directory API and specifically returns an object of type ChromeOSDevice so that name makes sense there. In both cases, the name is basically going off the actual API that Google provides, so any inconsistency is there. Here it wouldn't be wrong to call it, say, Get-GSChromeOSDeviceTelemetry but now you've made this command inconsistent with the API it uses (OS doesn't appear anywhere in the API names) to keep it consistent with some other basically unrelated API.

As an example of the inconsistencies that appear in something as large as Google's APIs, here's something I came across a few years ago: The enums for two separate endpoints are different in .Net even though they're the same in the REST documentation: googleapis/google-api-dotnet-client#1593

So, I'd say what you've done is fine. If anything, there could be an argument that there should be an alias of Get-GSChromeOSDeviceTelemetry pointing to this function, and Get-GSChromeDevice pointing to Get-GSChromeOSDevice but I don't feel like I'd know enough about Chrome Devices and their management to make that call.

Copy link
Collaborator

@FISHMANPET FISHMANPET left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, sorry, one more thing I noticed as I was looking at this. Normally I'd just test this myself and figure out the answer but since I don't have access to this in my org I have to rely on someone else to test.

PSGSuite/Public/Chrome/Get-GSChromeDeviceTelemetry.ps1 Outdated Show resolved Hide resolved
Added parameter $CustomerID
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants