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

feat: Support Copilot metrics endpoints #3350

Merged
merged 11 commits into from
Nov 20, 2024
Merged

Conversation

henriklundstrom
Copy link
Contributor

Introduces structs and functions for calling the newly GA endpoints for Copilot metrics. See this post from GitHub https://github.blog/changelog/2024-10-30-github-copilot-metrics-api-ga-release-now-available/.

Copy link

google-cla bot commented Nov 18, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

github/copilot.go Outdated Show resolved Hide resolved
github/copilot.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.33%. Comparing base (2b8c7fa) to head (01e652a).
Report is 175 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3350      +/-   ##
==========================================
- Coverage   97.72%   92.33%   -5.39%     
==========================================
  Files         153      176      +23     
  Lines       13390    15095    +1705     
==========================================
+ Hits        13085    13938     +853     
- Misses        215     1064     +849     
- Partials       90       93       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @henriklundstrom !
First, there are a few acronyms that need to be fixed for idiomatic Go naming conventions.

Then, before continuing my review, it seems like there is a lot of duplication in the new set of structs... are there structs (potential candidates are the ones utilizing omitempty) that could possibly be shared, or is there enough distinction to warrant the current number of structs?

I'm fine if the answer is the latter - I just wanted your opinion to see if you think we could take advantage of simplifying the interface by a bit of sharing (like we do with other structs). But again, if not, no big deal... then no changes necessary.

github/copilot.go Outdated Show resolved Hide resolved
github/copilot.go Outdated Show resolved Hide resolved
github/copilot.go Outdated Show resolved Hide resolved
github/copilot.go Outdated Show resolved Hide resolved
github/copilot.go Outdated Show resolved Hide resolved
github/copilot.go Outdated Show resolved Hide resolved
github/copilot.go Outdated Show resolved Hide resolved
github/copilot.go Outdated Show resolved Hide resolved
github/copilot.go Outdated Show resolved Hide resolved
github/copilot.go Outdated Show resolved Hide resolved
@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Nov 18, 2024
@henriklundstrom
Copy link
Contributor Author

Thank you, @henriklundstrom ! First, there are a few acronyms that need to be fixed for idiomatic Go naming conventions.

Then, before continuing my review, it seems like there is a lot of duplication in the new set of structs... are there structs (potential candidates are the ones utilizing omitempty) that could possibly be shared, or is there enough distinction to warrant the current number of structs?

I'm fine if the answer is the latter - I just wanted your opinion to see if you think we could take advantage of simplifying the interface by a bit of sharing (like we do with other structs). But again, if not, no big deal... then no changes necessary.

@gmlewis I've gone through your renaming suggestions and applied them all.

I agree there are many structs being introduced and that they're somewhat repetitive, but still not actually modular, and it's not a good read... Unfortunately this is how GitHub has structured the response schema. In this case I don't see that using struct embedding or reusable structs will make it any better. Possibly, I could introduce some:

type CopilotModelBase struct {
	Name                    string  `json:"name"`
	IsCustomModel           bool    `json:"is_custom_model"`
	CustomModelTrainingDate *string `json:"custom_model_training_date,omitempty"`
}

That would cut down the size of the CopilotIDECodeCompletionsModel, CopilotIDEChatModel, CopilotDotcomChatModel, and CopilotDotcomPullRequestsModel structs, but not by much and I'm not sure it makes them any nicer to work with.

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 19, 2024

I'm not sure it makes them any nicer to work with.

Thank you for double-checking... I appreciate it. Let's run with what you've got.

@gmlewis gmlewis mentioned this pull request Nov 19, 2024
@gmlewis
Copy link
Collaborator

gmlewis commented Nov 19, 2024

@henriklundstrom - to proceed with this PR, it looks like you need to do the following steps:

  • git checkout master
  • git pull upstream master (or however you set up your repo to pull from the upstream repo)
  • git checkout nordnet:copilot-metrics
  • git merge master
  • ./script/generate.sh (see step 4 of CONTRIBUTING.md for more details)
  • git push origin nordnet:copilot-metrics

This should pull #3352 into your PR which has the latest OpenAPI changes needed to support copilot-metrics.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @henriklundstrom !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

Copy link
Contributor

@claystation claystation left a comment

Choose a reason for hiding this comment

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

LGTM :)

@henriklundstrom
Copy link
Contributor Author

@gmlewis I've updated the branch and we have a second approval now 🚀

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Nov 20, 2024
@gmlewis
Copy link
Collaborator

gmlewis commented Nov 20, 2024

Thank you, @claystation !
Merging.

@gmlewis gmlewis merged commit 717e93f into google:master Nov 20, 2024
6 of 7 checks passed
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.

3 participants