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

Yoke metrics to app context - backend changes #1295

Closed
amurphy-cl opened this issue Feb 7, 2024 · 12 comments · Fixed by #1472
Closed

Yoke metrics to app context - backend changes #1295

amurphy-cl opened this issue Feb 7, 2024 · 12 comments · Fixed by #1472
Assignees
Labels

Comments

@amurphy-cl
Copy link
Collaborator

amurphy-cl commented Feb 7, 2024

Currently, when UpGrade is used with multiple app contexts, all metrics from all contexts are visible to the user. Metrics should be specific to the app context, and when defining metrics within an experiment, the user should not see metrics for different app contexts other than the one defined for a given experiment.

Add app context to metrics structure. Metrics will not share app contexts.

Notes from the meeting

  • Add new key to json structure that contains app-context
  • Backwards compatibility
    • Logging won’t check app context when importing old experiments
    • We will remove and redefine metrics according to context, since data is already there, rather than support backwards compatibility
  • Importing old experiments
    • Will error if metrics are not present in schema
    • Metrics queries are imported currently, will continue to import
@amurphy-cl amurphy-cl added the enhancement New feature or request label Feb 7, 2024
@amurphy-cl amurphy-cl added this to the Program Increment PI11 milestone Feb 7, 2024
@amurphy-cl amurphy-cl changed the title Yoke metrics to app context Yoke metrics to app context - backend changes Mar 18, 2024
@amurphy-cl
Copy link
Collaborator Author

@VivekFitkariwala any other refinement needed here?

@VivekFitkariwala
Copy link
Collaborator

This restriction should not be applied when adding logs because that will break the existing experiments.

@amurphy-cl this is good.

@ppratikcr7 ppratikcr7 moved this to Refined ToDo in UpGrade Project Mar 27, 2024
@ppratikcr7 ppratikcr7 assigned RidhamShah and unassigned ppratikcr7 Apr 4, 2024
@ppratikcr7 ppratikcr7 moved this from Refined ToDo to In Progress in UpGrade Project Apr 4, 2024
@VivekFitkariwala
Copy link
Collaborator

For the structure of the post Metrics we have two structures

{
MetricUnit : [{
 groupClass: 'groupClass1',
 allowedKeys: ['allowedKey1'],
 attributes: [{
   metric: 'metric1',
   key: 'key1',
   datatype: 'datatype1',
   allowedValues: ['allowedValue1']
 }]
}]
context: ['context1']
}

The context is common for all the Metrics

The second one

MetricUnit : [{
 groupClass: 'groupClass1',
 context: ['context1']
 allowedKeys: ['allowedKey1'],
 attributes: [{
   metric: 'metric1',
   key: 'key1',
   context: ['context1']
   datatype: 'datatype1',
   allowedValues: ['allowedValue1']
 }]
}]

The context is per Metrics

cc - @amurphy-cl @danoswaltCL @bcb37 @RidhamShah

@VivekFitkariwala
Copy link
Collaborator

VivekFitkariwala commented Apr 10, 2024

In the client lib should we update the addMetrics or are we going to remove this from client lib? Based on this issue

cc - @amurphy-cl @bcb37 @danoswaltCL @RidhamShah

@danoswaltCL
Copy link
Collaborator

My view is that we either have to remove it or expand it, just "add" isn't really enough for giving the client the tools to manage their metrics.

I think it may be simpler to remove this capability if there isn't a good use case for why this is easier than 'admin' CRUD operations, it's already a little odd to have 2 endpoints do exactly the same thing.

"Who" is authorized to change metrics state for that app-context, a google-authenticated individual user using a Google-Credential generated token, or a client-application system user using our API Token?

@amurphy-cl
Copy link
Collaborator Author

amurphy-cl commented Apr 11, 2024

Decision: use first structure so metrics will be exclusive to context. Also will remove addMetrics from the client library.

@RidhamShah
Copy link
Collaborator

RidhamShah commented Apr 12, 2024

@amurphy-cl @danoswaltCL Do we want to use pre-defined metrics from the .env file. If yes, then we may need to update it as it should contain context now.

@danoswaltCL
Copy link
Collaborator

yeah it's definitely useful for dev/testing at least so we don't have to reload metrics when destroying dbs, so let's update it. We'll update in our env configs.

@RidhamShah RidhamShah moved this from In Progress to Code Review in UpGrade Project Apr 25, 2024
@RidhamShah RidhamShah linked a pull request May 2, 2024 that will close this issue
@RidhamShah
Copy link
Collaborator

The new metrics structure of env file

METRICS = [
     {
          metrics: [
               {
                    metric: totalTimeSeconds, 
                    datatype: 'continuous'
               },
               {
                    metric: 'workspaceCompletionStatus',
                    ...
               },
               {
                    groupClass: 'addWorkspace',
                    allowedKeys: ['level1', 'level2'],
                    ...
               }
          ],
          contexts: [ context1, context2 ]
     },
     {
          metrics: [ ... ]
          contexts: [ ... ]
     }
]

@ppratikcr7 ppratikcr7 moved this from Code Review to QA in UpGrade Project Jun 3, 2024
@ppratikcr7 ppratikcr7 assigned ppratikcr7 and unassigned RidhamShah Sep 25, 2024
@ppratikcr7
Copy link
Collaborator

@danoswaltCL QA: Seems to work as expected. The context chip based click and search doesn't seem to work. We do have this feature in Experiment, Segment & Feature Flag Pages. Should we create another ticket for this?

@ppratikcr7
Copy link
Collaborator

Creating a separate ticket for the above issue.

@ppratikcr7 ppratikcr7 moved this from QA to Done in UpGrade Project Sep 27, 2024
@ppratikcr7
Copy link
Collaborator

QA passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants