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

Remove addMetrics from client library? #1332

Closed
danoswaltCL opened this issue Feb 28, 2024 · 5 comments · Fixed by #1662
Closed

Remove addMetrics from client library? #1332

danoswaltCL opened this issue Feb 28, 2024 · 5 comments · Fixed by #1662
Assignees
Labels
enhancement New feature or request priority: low Low priority issue release 6.0

Comments

@danoswaltCL
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
The addMetrics method is a little out of place in the client library, as it is not tied to user actions in the client, and is an administrative task that would normally require a different kind of authorization. This is a little confusing and incomplete anyway, as one cannot do other CRUD operations other than add.

Describe the solution you'd like

  1. Remove method from libraries.
  2. Remove endpoint from experiment client controller
  3. Unify the CRUD operations for metrics so they are all grouped under one controller and one swagger section

Alternatively, we could decide that metrics are fine as a client-initiated task, in which case we should give the client libraries full CRUD access instead of just add.

@danoswaltCL danoswaltCL added the enhancement New feature or request label Feb 28, 2024
@amurphy-cl
Copy link
Collaborator

This exists because of a request from @SritterCL, because the MATHia server sends which metrics can be collected, and it's done through the client-side SDK. Aside from this case (implemented but not used), it doesn't need to be in the client SDK.

@amurphy-cl amurphy-cl added the priority: low Low priority issue label Mar 5, 2024
@amurphy-cl amurphy-cl moved this to Refined ToDo in UpGrade Project Mar 5, 2024
@amurphy-cl amurphy-cl added this to the Program Increment PI11 milestone Apr 11, 2024
@RidhamShah RidhamShah self-assigned this Jun 6, 2024
@RidhamShah RidhamShah moved this from Refined ToDo to In Progress in UpGrade Project Jun 6, 2024
@RidhamShah RidhamShah moved this from In Progress to Code Review in UpGrade Project Jun 14, 2024
@RidhamShah RidhamShah linked a pull request Jun 14, 2024 that will close this issue
@VivekFitkariwala
Copy link
Collaborator

@danoswaltCL @amurphy-cl Should the endpoint be removed from the latest version or all the versions?
cc - @JayMehta11

@danoswaltCL
Copy link
Collaborator Author

just v5, we should make a proper deprecation announcement in documentation that 4 and lower are not officially supported. I don't know if we should ever be modifying old controller versions unless there's something majorly wrong, we have retroactively added new features to old versions before but now that everyone at CL is on the latest major version, we can really drop these old controller versions entirely.

@JayMehta11
Copy link
Contributor

I have removed the endpoint in v5, and I have added the code back in the older versions as discussed.

@RidhamShah RidhamShah moved this from Code Review to QA in UpGrade Project Jun 24, 2024
@RidhamShah RidhamShah linked a pull request Jun 24, 2024 that will close this issue
@RidhamShah RidhamShah assigned JayMehta11 and unassigned RidhamShah Jun 24, 2024
@RidhamShah RidhamShah moved this from QA to Code Review in UpGrade Project Jun 24, 2024
@ppratikcr7 ppratikcr7 moved this from Code Review to QA in UpGrade Project Jun 25, 2024
@danoswaltCL
Copy link
Collaborator Author

QA passed

@danoswaltCL danoswaltCL self-assigned this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: low Low priority issue release 6.0
Projects
Status: Done
5 participants