Metrics Fixes #2319

maroshii · 2022-03-06T00:12:02Z

Send the deploy origin in the deploy mutation. Adding a special source: okteto-deploy used for deploy commands ran from within another okteto deploy

derek · 2022-03-06T00:12:05Z

Thank you for your contribution. unfortunately, one or more of your commits are missing the required "Signed-off-by:" statement. Signing off is part of the Developer Certificate of Origin (DCO) which is used by this project.

Read the DCO and project contributing guide carefully, and amend your commits using the git CLI. Note that this does not require any cryptography, keys or special steps to be taken.

💡 Shall we fix this?

This will only take a few moments.

First, clone your fork and checkout this branch using the git CLI.

Next, set up your real name and email address:

git config --global user.name "Your Full Name"
git config --global user.email "you@domain.com"

Finally, run one of these commands to add the "Signed-off-by" line to your commits.

If you only have one commit so far then run: git commit --amend --signoff and then git push --force.
If you have multiple commits, watch this video.

Check that the message has been added properly by running "git log".

codecov-commenter · 2022-03-06T00:20:28Z

Codecov Report

Merging #2319 (289c304) into master (fea9900) will decrease coverage by 0.10%.
The diff coverage is 1.21%.

@@            Coverage Diff             @@
##           master    #2319      +/-   ##
==========================================
- Coverage   32.05%   31.94%   -0.11%     
==========================================
  Files         149      149              
  Lines       17133    17194      +61     
==========================================
+ Hits         5492     5493       +1     
- Misses      11009    11069      +60     
  Partials      632      632

Impacted Files	Coverage Δ
pkg/analytics/track.go	`5.98% <0.00%> (-0.27%)`	⬇️
pkg/config/config.go	`18.75% <0.00%> (-1.64%)`	⬇️
pkg/okteto/context.go	`18.72% <0.00%> (ø)`
pkg/okteto/pipeline.go	`0.00% <0.00%> (ø)`
pkg/okteto/preview.go	`0.00% <0.00%> (ø)`
cmd/deploy/deploy.go	`24.39% <4.54%> (-0.71%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fea9900...289c304. Read the comment docs.

jLopezbarb · 2022-03-07T09:15:42Z

pkg/okteto/pipeline.go

@@ -58,6 +61,7 @@ func (c *OktetoClient) DeployPipeline(ctx context.Context, name, repository, bra
 			"branch":     graphql.String(branch),
 			"variables":  variablesVariable,
 			"filename":   graphql.String(filename),
+			"source":     graphql.String(origin),
 		}

 		err := mutate(ctx, &mutation, queryVariables, c.client)


Should we check if the mutation returns the new error "Unknown argument \"source\" on type \"GitDeploy\"") so we can run the deprecated version of deployPipeline

yes, I think we should do it because the source parameter was released on the OE 0.10.7 and probably there are still OE in previous versions. @pchico83 Can you confirm is there is any OE in version < 0.10.7?

yes, this change should be backward compatible. One option is to pass this value as an envvar of the pipeline deployment, not sure if that makes things easier or not

Got it. If we use the deprecatedDeploy tho it would send all deploys on 0.10.7 to the deprecated path of 0.10.0, right? I'll try to send it as an envvar as Pablo suggests

ifbyol · 2022-03-07T09:29:12Z

pkg/config/config.go

+	}
+	// deploys within another okteto deploy take precedence as a deploy origin.
+	// This is running okteto pipeline deploy as a step of another okteto deploy
+	if os.Getenv(model.OktetoWithinDeployCommandContextEnvVar) == "true" {


Maybe we need to revisit the places where we are checking this env var. Deploy command sets it as false almost at the beginning of the command: https://github.com/okteto/okteto/blob/master/cmd/deploy/deploy.go#L131

I see several checks that I'm not sure they are working as expected

This works for multi repo with okteto pipeline deploy. Eg: https://github.com/okteto/movies-api/blob/main/okteto-pipeline.yml#L3

Not sure if okteto deploy inside another okteto deploy is a thing? If so the metrics will be somewhat skewed but not sure how many of these occurrences there will be. I can always create another envvar

I don't think it will be frequent. For now I wouldn't consider that case until users start using it.

There is one case that I'm not sure which should be the right value for the metric. If you execute okteto deploy -b master or okteto deploy -r https://github.com/okteto/movies, it will execute directly a pipeline https://github.com/okteto/okteto/blob/master/cmd/deploy/deploy.go#L116 but it wouldn't be considered within an okteto deploy command. Which honestly, not sure if it is right or not.

Also, bear in mind that a pipeline is executed in the backend as an okteto deploy, in case it affects somehow the metrics

@ifbyol can you take another look? I added OKTETO_SKIP_CONFIG_CREDENTIALS_UPDATE for the proxy case you describe and kept the origin OKTETO_WITHIN_DEPLOY_COMMAND_CONTEXT independent from all that so we can use it safely everywhere else knowing it will not change

@maroshii looks good to me, but there are other places where that variable was using before and not sure if they should use the new one or the old one. Looking in to the code, I would say they have to use OKTETO_WITHIN_DEPLOY_COMMAND_CONTEXT, but @jLopezbarb can you double check, please? If you have doubts about what we are talking about, let me know and we can do a quick call

ifbyol · 2022-03-07T09:39:34Z

pkg/okteto/pipeline.go

@@ -58,6 +61,7 @@ func (c *OktetoClient) DeployPipeline(ctx context.Context, name, repository, bra
 			"branch":     graphql.String(branch),
 			"variables":  variablesVariable,
 			"filename":   graphql.String(filename),
+			"source":     graphql.String(origin),
 		}

 		err := mutate(ctx, &mutation, queryVariables, c.client)


yes, I think we should do it because the source parameter was released on the OE 0.10.7 and probably there are still OE in previous versions. @pchico83 Can you confirm is there is any OE in version < 0.10.7?

maroshii · 2022-03-07T20:18:13Z

cmd/deploy/deploy.go

+				Duration:               time.Since(startTime),
+				PipelineType:           c.PipelineType,
+				DeployType:             deployType,
+				IsPreview:              false, // TODO: How should we get this value?


@jLopezbarb is there a way to know this here?

The only thing I can think of is checking if its an okteto cluster and then call to the API to check if exists the namespace as preview or namespace

Possible options I see are:

Use client-go library to get the ns information and check annotations/labels (scope). I don't like it because we would be coupling the CLI with the logic of checking the scope to know if it is a preview env or a normal namespace and it would be harder to remove the scope in the future

Pass an environment variable to the pipeline job specifying if it is a normal pipeline or a preview environment. In that way, that environment variable would be injected in the okteto deploy command and it would be available here to check

maroshii · 2022-03-07T20:19:09Z

NOTE TO SELF: we are still missing to track and send OKTETO_ORIGIN as a variable in the deployPreview mutation

ifbyol · 2022-03-08T10:25:36Z

cmd/deploy/deploy.go

+				Duration:               time.Since(startTime),
+				PipelineType:           c.PipelineType,
+				DeployType:             deployType,
+				IsPreview:              false, // TODO: How should we get this value?


Possible options I see are:

Use client-go library to get the ns information and check annotations/labels (scope). I don't like it because we would be coupling the CLI with the logic of checking the scope to know if it is a preview env or a normal namespace and it would be harder to remove the scope in the future

Pass an environment variable to the pipeline job specifying if it is a normal pipeline or a preview environment. In that way, that environment variable would be injected in the okteto deploy command and it would be available here to check

ifbyol · 2022-03-08T10:26:08Z

pkg/analytics/track.go

+// TrackDeploy sends a tracking event to mixpanel when the user deploys a pipeline
+func TrackDeploy(m TrackDeployMetadata) {
+	// skip deploys from nested okteto deploys and manifest dependencies
+	if config.GetDeployOrigin() == "okteto-deploy" {


maroshii · 2022-03-08T11:07:37Z

pkg/analytics/track.go

@@ -49,7 +49,7 @@ const (
 	doctorEvent              = "Doctor"
 	buildEvent               = "Build"
 	buildTransientErrorEvent = "BuildTransientError"
-	deployEvent              = "Deploy"
+	deployEvent              = "OktetoDeploy"


@rlamana this is the new event name. Between this change and the recent backend change we'll no longer get the Deploy event. Just confirming we are ok with this

So, is this the event for the nested ones? Or in general? If we end up starting a new mixpanel project like we've been discussing, I want this event to still be Deploy. And if we end up using a prefix, I think all these events (UP, CONTEXT, etc..) should have the same prefix.

pkg/analytics/track.go

maroshii · 2022-03-09T17:02:54Z

With the latest changes this PR tracks all the metrics needed as per business requirements.
It takes into account OKTETO_ORIGIN from other sources like vscode, github actions, etc. This wasn't properly forwarded for previews for eg.

ifbyol

LGTM with the context I have

maroshii requested a review from a team March 6, 2022 00:12

derek bot added the no-dco label Mar 6, 2022

jLopezbarb reviewed Mar 7, 2022

View reviewed changes

ifbyol reviewed Mar 7, 2022

View reviewed changes

maroshii added 3 commits March 7, 2022 17:14

Add deploy origin

023766f

send OKTETO_ORIGIN as deploy variable

708b9fd

Add OKTETO_SKIP_CONFIG_CREDENTIALS_UPDATE envvar

e70177b

maroshii force-pushed the fran/deploy-origin branch 2 times, most recently from ea9264b to 02ef72d Compare March 7, 2022 20:16

fixes

86e2d2a

maroshii force-pushed the fran/deploy-origin branch from 02ef72d to 86e2d2a Compare March 7, 2022 20:17

maroshii commented Mar 7, 2022

View reviewed changes

ifbyol reviewed Mar 8, 2022

View reviewed changes

rename deploy event

6d7af33

maroshii commented Mar 8, 2022

View reviewed changes

maroshii added 3 commits March 8, 2022 17:13

track isPreview

881c385

Fix variables

983cffc

add hasBuildSection

7678579

maroshii changed the title ~~Add deploy origin~~ Track Deploy Mar 9, 2022

maroshii added 2 commits March 9, 2022 10:04

Rename to UnifiedDeploy

536a5cb

use OKTETO_ORIGIN for preview deploys

289c304

maroshii commented Mar 9, 2022

View reviewed changes

pkg/analytics/track.go Outdated Show resolved Hide resolved

track up and auto deploy

b3ad246

maroshii changed the title ~~Track Deploy~~ Metrics Fixes Mar 9, 2022

revert mixpanel token

30334bb

maroshii requested a review from rlamana March 9, 2022 17:02

maroshii requested review from pchico83, jLopezbarb and ifbyol March 9, 2022 17:03

ifbyol approved these changes Mar 9, 2022

View reviewed changes

pchico83 approved these changes Mar 10, 2022

View reviewed changes

maroshii merged commit 9ab9c9d into master Mar 11, 2022

maroshii deleted the fran/deploy-origin branch March 11, 2022 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metrics Fixes #2319

Metrics Fixes #2319

maroshii commented Mar 6, 2022

derek bot commented Mar 6, 2022

codecov-commenter commented Mar 6, 2022 •

edited

jLopezbarb Mar 7, 2022

ifbyol Mar 7, 2022

pchico83 Mar 7, 2022

maroshii Mar 7, 2022

ifbyol Mar 7, 2022

maroshii Mar 7, 2022

ifbyol Mar 7, 2022

maroshii Mar 7, 2022

ifbyol Mar 7, 2022

ifbyol Mar 7, 2022

maroshii Mar 7, 2022

jLopezbarb Mar 8, 2022

ifbyol Mar 8, 2022

maroshii commented Mar 7, 2022

ifbyol Mar 8, 2022

ifbyol Mar 8, 2022

maroshii Mar 8, 2022

rlamana Mar 8, 2022

maroshii commented Mar 9, 2022

ifbyol left a comment

Metrics Fixes #2319

Metrics Fixes #2319

Conversation

maroshii commented Mar 6, 2022

derek bot commented Mar 6, 2022

💡 Shall we fix this?

codecov-commenter commented Mar 6, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maroshii commented Mar 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maroshii commented Mar 9, 2022

ifbyol left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 6, 2022 •

edited