Add lakefs instrumentation #6217

N-o-Z · 2023-07-17T12:35:10Z

Linked Issue

Closes #6216

github-actions · 2023-07-17T12:36:06Z

🎊 PR Preview 0268f29 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-6217.surge.sh

🕐 Build time: 0.018s

_{🤖 By surge-preview}

nopcoder · 2023-07-17T13:19:59Z

pkg/auth/metadata.go

+	_, k8s := os.LookupEnv("KUBERNETES_SERVICE_HOST")
+	lakefsAccessKeyID := os.Getenv("LAKEFS_ACCESS_KEY_ID")
+	quickstart, _ := strconv.ParseBool(os.Getenv("QUICKSTART"))
+	_, statErr := StatFunc("/.dockerenv")


separate keys for docker / k8s / sample|quickstart

stats - global string point to dockerenv filename instead of override stat func

nopcoder · 2023-07-17T13:20:49Z

.github/workflows/docker-publish.yaml

+            treeverse/lakefs:latest
+
+      - name: Build and push lakefs quickstart


if we will get to the point where we will need to version quickstart images I prefer to keep it simple and think of alternative way to identify quickstart usage - arg? or just env var and not new image.

rmoff

I'm not in favour of a new Docker image for quickstart, sorry. This is for two reasons:

Quickstart should ideally be a "runway" for users to get up and running (and take off, to continue the analogy)—using a dedicated image is like a dead-end road. There are exceptions to this rule but they need to be a good one, not just to make it easier to instrument
If we use a dedicated image we need to explain to the user why, and that explanation is too thin to hold weight IMHO

N-o-Z · 2023-07-17T13:45:06Z

I'm not in favour of a new Docker image for quickstart, sorry. This is for two reasons:

Quickstart should ideally be a "runway" for users to get up and running (and take off, to continue the analogy)—using a dedicated image is like a dead-end road. There are exceptions to this rule but they need to be a good one, not just to make it easier to instrument

If we use a dedicated image we need to explain to the user why, and that explanation is too thin to hold weight IMHO

Appreciate the feedback, please suggest an alternative for identifying execution as part of the quickstart guide

nopcoder · 2023-07-17T13:47:42Z

pkg/auth/metadata.go

@@ -206,13 +212,43 @@ func (m *KVMetadataManager) IsInitialized(ctx context.Context) (bool, error) {
 	return !setupTimestamp.IsZero(), nil
 }

+// DockerFileExists For testing purposes
+var DockerFileExists = "/.dockerenv"


DockeEnvExists

nopcoder · 2023-07-17T14:01:38Z

pkg/auth/metadata.go

 func (m *KVMetadataManager) GetMetadata(ctx context.Context) (map[string]string, error) {
 	metadata := make(map[string]string)
 	metadata["lakefs_version"] = m.version
 	metadata["lakefs_kv_type"] = m.kvType
 	metadata["golang_version"] = runtime.Version()
 	metadata["architecture"] = runtime.GOARCH
 	metadata["os"] = runtime.GOOS
+	getInstrumentation(metadata)


Suggest to split into functions

Suggested change

getInstrumentation(metadata)

metadata["is_k8s"] = inK8sMetadata()

metadata["is_docker"] = inDockerMetadata()

metadata["instrumentation"] = instrumentationMetadata()

func isK8sMetadata() string { _, k8s := os.LookupEnv("KUBERNETES_SERVICE_HOST") return strconv.FormatBool(k8s) } func isDockerMetadata() string { var err error if DockerFileExists != "" { _, err = os.Stat(DockerFileExists) } return strconv.FormatBool(err == nil) } func instrumentationMetadata() string { lakefsAccessKeyID := os.Getenv("LAKEFS_ACCESS_KEY_ID") if strings.HasSuffix(lakefsAccessKeyID, "LKFSSAMPLES") { return InstrumentationSamplesRepo } quickstart, _ := strconv.ParseBool(os.Getenv("QUICKSTART")) if quickstart { return InstrumentationQuickstart } return InstrumentationRun }

nopcoder

Thanks! looks good - if there is an alternative for the docker image I'm +1

N-o-Z · 2023-07-18T11:51:58Z

@nopcoder Please re-review quickstart refactor

nopcoder · 2023-07-18T12:27:38Z

pkg/auth/metadata.go

+	switch {
+	case strings.HasSuffix(lakefsAccessKeyID, "LKFSSAMPLES"):
 		return InstrumentationSamplesRepo
-	}
-	quickstart, _ := strconv.ParseBool(os.Getenv("QUICKSTART"))
-	if quickstart {
+	case strings.HasSuffix(lakefsAccessKeyID, "QUICKSTART"):
 		return InstrumentationQuickstart
+	default:
+		return InstrumentationRun
 	}
-	return InstrumentationRun
 }


nopcoder · 2023-07-18T12:46:19Z

cmd/lakefs/cmd/root.go

+		os.Exit(1)
+	}
+	accessKey := "AKIAIOSFOLQUICKSTART"
+	secretKey := "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"


will need to lint ignore - gosec

nopcoder · 2023-07-18T12:46:59Z

cmd/lakefs/cmd/root.go

+	accessKey := "AKIAIOSFOLQUICKSTART"
+	secretKey := "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"


Suggested change

accessKey := "AKIAIOSFOLQUICKSTART"

secretKey := "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"

const (

accessKey = "AKIAIOSFOLQUICKSTART"

secretKey = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"

)

nopcoder · 2023-07-18T12:47:26Z

cmd/lakefs/cmd/root.go

+	fmt.Printf("Access Key ID    : %s \n", accessKey)
+	fmt.Printf("Secret Access Key: %s \n", secretKey)


Suggested change

fmt.Printf("Access Key ID : %s \n", accessKey)

fmt.Printf("Secret Access Key: %s \n", secretKey)

fmt.Printf("Access Key ID : %s\n", accessKey)

fmt.Printf("Secret Access Key: %s\n", secretKey)

nopcoder · 2023-07-18T12:54:08Z

cmd/lakefs/cmd/root.go

-	if useLocal() {
-		return config.NewLocalConfig()
-	}
+	quickStart := useConfig(config.QuickStartConfiguration)


Do we need to verify that not both are on?

Not sure we wan't to enforce mutual exclusion - do you see an issue with it?

nopcoder · 2023-07-18T12:57:08Z

cmd/lakefs/cmd/root.go

 }

-func useLocal() bool {
-	res, err := rootCmd.PersistentFlags().GetBool(config.UseLocalConfiguration)
+func validateQuickstartEnv(cfg *config.Config) {


The name is a bit misleading as we verify the database and blockstore - but we override the configuration and not set them as default.
If we like to verify that the default as not override we should do the same as local - setup the right defaults for quick-start and verify after the configuration is loaded that the user didn't override the default for all the values we require.

nopcoder · 2023-07-18T14:25:35Z

cmd/lakefs/cmd/root.go

+	fmt.Printf("Secret Access Key: %s\n", config.DefaultQuickstartSecretKey)
+}
+
+func useConfig(cfg string) bool {


Suggested change

func useConfig(cfg string) bool {

func useConfig(flagName string) bool {

nopcoder · 2023-07-18T14:40:32Z

cmd/lakefs/cmd/root.go

-	if useLocal() {
-		return config.NewLocalConfig()
-	}
+	quickStart := useConfig(config.QuickstartConfiguration)


Suggest, if it is more clear and less references of calling NewConfig

name := "" configurations := []string{config.QuickstartConfiguration, config.UseLocalConfiguration} if idx := slices.IndexFunc(configurations, func(f string) bool { return useConfig(f); }); idx != -1 { name = configurations[idx] } cfg, err := config.NewConfig(name) if err != nil { return nil, err } if name == config.QuickstartConfiguration { validateQuickstartEnv(cfg) } return cfg, nil

nopcoder · 2023-07-18T14:41:06Z

pkg/config/defaults.go

-func setDefaults(local bool) {
-	if local {
+func setDefaults(cfgType string) {
+	switch {


Suggested change

switch {

switch cfgType {

nopcoder

LGTM!

rmoff

Firstly: I think this is a positive change and a good direction 👍🏻

However 😬 I have some nits, as well as a blocker.

🛑 Potential blocker: This change will remove quickstart email capture

From a DevEx point of view I am 💯 💯 💯 💯 💯 in favour of this, but last time I did it it caused ructions elsewhere ;)

We need to make it clear to all stakeholders (@iddoavn, @einato , @ozkatz et al) the impact and get agreement from them.

The solution here is to implement #6033 / #6204 first.

Nits

Pre-create the quickstart repo

Since we know they're running the quickstart, we should pre-create the quickstart sample repo

Sample repo instructions

These need updating for the new --quickstart flag

Startup Banner

The startup banner should reflect the startup mode; if the user is using --quickstart then echo the pre-baked creds to stdout and not direct them to complete the setup process at http://127.0.0.1:8000/setup

You already have this change but only in the docs, whereas it should be conditional and in the lakeFS output itself

i.e. instead the current startup banner:


     ██╗      █████╗ ██╗  ██╗███████╗███████╗███████╗
     ██║     ██╔══██╗██║ ██╔╝██╔════╝██╔════╝██╔════╝
     ██║     ███████║█████╔╝ █████╗  █████╗  ███████╗
     ██║     ██╔══██║██╔═██╗ ██╔══╝  ██╔══╝  ╚════██║
     ███████╗██║  ██║██║  ██╗███████╗██║     ███████║
     ╚══════╝╚═╝  ╚═╝╚═╝  ╚═╝╚══════╝╚═╝     ╚══════╝

│
│ If you're running lakeFS locally for the first time,
│     complete the setup process at http://127.0.0.1:8000/setup
│

│
│ For more information on how to use lakeFS,
│     check out the docs at https://docs.lakefs.io/quickstart/
│

│
│ For support or any other question,                            >(.＿.)<
│     join our Slack channel https://docs.lakefs.io/slack         (  )_
│

Should be this for quickstart:


     ██╗      █████╗ ██╗  ██╗███████╗███████╗███████╗
     ██║     ██╔══██╗██║ ██╔╝██╔════╝██╔════╝██╔════╝
     ██║     ███████║█████╔╝ █████╗  █████╗  ███████╗
     ██║     ██╔══██║██╔═██╗ ██╔══╝  ██╔══╝  ╚════██║
     ███████╗██║  ██║██║  ██╗███████╗██║     ███████║
     ╚══════╝╚═╝  ╚═╝╚═╝  ╚═╝╚══════╝╚═╝     ╚══════╝

│
│ You are running lakeFS in quickstart mode. 
|   Login at http://127.0.0.1:8000/
│     
|    Access Key ID    : AKIAIOSFOLQUICKSTART
│    Secret Access Key: wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY
│

│
│ For more information on how to use lakeFS,
│     check out the docs at https://docs.lakefs.io/quickstart/
│

│
│ For support or any other question,                            >(.＿.)<
│     join our Slack channel https://docs.lakefs.io/slack         (  )_
│

I realise that the creds are echoed earlier on as part of the bootup but from a DX point of view it's not obvious particularly when there is a user-facing dialog later on in the stdout

N-o-Z · 2023-07-19T10:48:38Z

Firstly: I think this is a positive change and a good direction 👍🏻

However grimacing I have some nits, as well as a blocker.

Potential blocker: This change will remove quickstart email capture

From a DevEx point of view I am 100 100 100 100 100 in favour of this, but last time I did it it caused ructions elsewhere ;)

We need to make it clear to all stakeholders (@iddoavn, @einato , @ozkatz et al) the impact and get agreement from them.

The solution here is to implement #6033 / #6204 first.

Nits

Pre-create the quickstart repo

Since we know they're running the quickstart, we should pre-create the quickstart sample repo

Sample repo instructions

These need updating for the new --quickstart flag

Startup Banner

The startup banner should reflect the startup mode; if the user is using --quickstart then echo the pre-baked creds to stdout and not direct them to complete the setup process at http://127.0.0.1:8000/setup

You already have this change but only in the docs, whereas it should be conditional and in the lakeFS output itself

i.e. instead the current startup banner:
     ██╗      █████╗ ██╗  ██╗███████╗███████╗███████╗
     ██║     ██╔══██╗██║ ██╔╝██╔════╝██╔════╝██╔════╝
     ██║     ███████║█████╔╝ █████╗  █████╗  ███████╗
     ██║     ██╔══██║██╔═██╗ ██╔══╝  ██╔══╝  ╚════██║
     ███████╗██║  ██║██║  ██╗███████╗██║     ███████║
     ╚══════╝╚═╝  ╚═╝╚═╝  ╚═╝╚══════╝╚═╝     ╚══════╝

│
│ If you're running lakeFS locally for the first time,
│     complete the setup process at http://127.0.0.1:8000/setup
│

│
│ For more information on how to use lakeFS,
│     check out the docs at https://docs.lakefs.io/quickstart/
│

│
│ For support or any other question,                            >(.＿.)<
│     join our Slack channel https://docs.lakefs.io/slack         (  )_
│
Should be this for quickstart:
     ██╗      █████╗ ██╗  ██╗███████╗███████╗███████╗
     ██║     ██╔══██╗██║ ██╔╝██╔════╝██╔════╝██╔════╝
     ██║     ███████║█████╔╝ █████╗  █████╗  ███████╗
     ██║     ██╔══██║██╔═██╗ ██╔══╝  ██╔══╝  ╚════██║
     ███████╗██║  ██║██║  ██╗███████╗██║     ███████║
     ╚══════╝╚═╝  ╚═╝╚═╝  ╚═╝╚══════╝╚═╝     ╚══════╝

│
│ You are running lakeFS in quickstart mode. 
|   Login at http://127.0.0.1:8000/
│     
|    Access Key ID    : AKIAIOSFOLQUICKSTART
│    Secret Access Key: wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY
│

│
│ For more information on how to use lakeFS,
│     check out the docs at https://docs.lakefs.io/quickstart/
│

│
│ For support or any other question,                            >(.＿.)<
│     join our Slack channel https://docs.lakefs.io/slack         (  )_
│
I realise that the creds are echoed earlier on as part of the bootup but from a DX point of view it's not obvious particularly when there is a user-facing dialog later on in the stdout

Thanks,

Regarding the blocker:
As you stated - this will be resolved in #6204, so we can wait until that issue is resolved before merging this one - in that case there's no impact so there's nothing to discuss.

Nits:
Pre-create the quickstart repo
That's a good idea - but outside of this PR scope. We can open an issue for that if we want this feature

Startup Banner
Since we expected users running quickstart as part of the quickstart tutorial, we can assume they will continue to the next steps where we added the credentials.
Customizing the banner is a good idea, but I think the solution we have is good enough for now

rmoff · 2023-07-19T10:54:34Z

I'd like both the nits addressed as part of this PR please. If we're implementing a --quickstart flag (which is a great idea) then it should behave as a proper quickstart, not a partial one nor rely on users to RTFM ;)

N-o-Z · 2023-07-19T11:58:26Z

@rmoff Added the banner changes - but I have to insist on the sample repo creation. This PR should focus on the issue at hand which is providing a way to understand where lakefs is run from.
The quickstart flag was created out of necessity and not out of requirement. If we think we should add the creation of a sample repo as part of the "quickstart mode" we should discuss and do it as part of a designated context (issue)

rmoff

LGTM! Thanks for this @N-o-Z.

Assuming we wait for #6204 then we need to make sure that includes changes to the quickstart doc also as the flow will change there.

nopcoder · 2023-07-19T15:06:38Z

cmd/lakefs/cmd/run.go

+		}
+		templ := template.New("banner")
+		var buf bytes.Buffer
+		t := template.Must(templ.Parse(runBannerTmpl))


you can define var outside and use template must api to render it once

nopcoder · 2023-07-20T07:08:04Z

cmd/lakefs/cmd/run.go

+		}
+		if isQuickstart {
+			data.LocalSetup = ""
+			data.Quickstart = quickStartBanner


This should be done inside the template base on the args we provide.
provide the params we require to render the template - it can include the admin key/secret, the mode and the lakefs version.

nopcoder · 2023-07-20T07:08:32Z

cmd/lakefs/cmd/run.go

+			_, _ = fmt.Fprintf(os.Stderr, "Failed formatting banner: %v\n", err)
+			os.Exit(1)
+		}
+		_, _ = fmt.Fprintf(os.Stderr, "lakeFS %s - Up and running (^C to shutdown)...\n", version.Version)


nice - I think it can be also move inside the template

…trumentation-6216 # Conflicts: # docs/quickstart/launch.md

…trumentation-6216

guy-har

looked at the CR Fixes
LGTM

On vacation - Guy covered requested changes

Add lakefs instrumentation

bca631f

N-o-Z added BI exclude-changelog PR description should not be included in next release changelog labels Jul 17, 2023

N-o-Z requested review from nopcoder and rmoff July 17, 2023 12:35

N-o-Z self-assigned this Jul 17, 2023

nopcoder suggested changes Jul 17, 2023

View reviewed changes

rmoff suggested changes Jul 17, 2023

View reviewed changes

CR Fixes 1

6858929

N-o-Z requested a review from nopcoder July 17, 2023 13:46

nopcoder reviewed Jul 17, 2023

View reviewed changes

CR Fixes 2

297ae9e

N-o-Z requested a review from nopcoder July 17, 2023 14:00

nopcoder suggested changes Jul 17, 2023

View reviewed changes

CR Fixes 3

d18948b

N-o-Z requested a review from nopcoder July 17, 2023 14:13

nopcoder approved these changes Jul 17, 2023

View reviewed changes

Quickstart refactor

ec65f50

N-o-Z requested review from rmoff and nopcoder July 18, 2023 11:51

nopcoder reviewed Jul 18, 2023

View reviewed changes

Fix gosec

a54092a

nopcoder suggested changes Jul 18, 2023

View reviewed changes

N-o-Z added 2 commits July 18, 2023 16:29

NewConfig

99da604

gosec

258500b

N-o-Z requested a review from nopcoder July 18, 2023 13:34

Fix config

0715d58

nopcoder suggested changes Jul 18, 2023

View reviewed changes

Fix config 2

b610d8e

N-o-Z requested a review from nopcoder July 18, 2023 14:50

N-o-Z added 5 commits July 18, 2023 17:58

Fix lint

0d19140

Fix config var

534a501

lint

4f2122d

fix unit test

fa0bed9

fix unit test

c666bf7

nopcoder approved these changes Jul 18, 2023

View reviewed changes

rmoff suggested changes Jul 19, 2023

View reviewed changes

Banner modification

9eedc7e

N-o-Z requested a review from rmoff July 19, 2023 11:58

Doc nits

e9b3ae9

rmoff approved these changes Jul 19, 2023

View reviewed changes

rmoff mentioned this pull request Jul 19, 2023

lakeFS should pre-create the quickstart repository when it's launched with --quickstart #6232

Closed

nopcoder previously requested changes Jul 20, 2023

View reviewed changes

N-o-Z added 4 commits July 30, 2023 17:09

Merge remote-tracking branch 'origin/master' into task/add-lakefs-ins…

acce1d0

…trumentation-6216 # Conflicts: # docs/quickstart/launch.md

CR Fixes

25bd1c9

Merge branch 'master' into task/add-lakefs-instrumentation-6216

9c05160

Merge remote-tracking branch 'origin/master' into task/add-lakefs-ins…

0268f29

…trumentation-6216

N-o-Z requested a review from guy-har August 7, 2023 12:57

guy-har approved these changes Aug 7, 2023

View reviewed changes

N-o-Z merged commit e31c75c into master Aug 7, 2023
33 checks passed

N-o-Z deleted the task/add-lakefs-instrumentation-6216 branch August 7, 2023 13:33

rmoff mentioned this pull request Aug 15, 2023

[Bug]: Quickstart is broken in v0.106.0 #6400

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lakefs instrumentation #6217

Add lakefs instrumentation #6217

N-o-Z commented Jul 17, 2023

github-actions bot commented Jul 17, 2023 •

edited

Loading

nopcoder Jul 17, 2023

nopcoder Jul 17, 2023

rmoff left a comment

N-o-Z commented Jul 17, 2023

nopcoder Jul 17, 2023

nopcoder Jul 17, 2023

nopcoder left a comment

N-o-Z commented Jul 18, 2023

nopcoder Jul 18, 2023

nopcoder Jul 18, 2023

nopcoder Jul 18, 2023

nopcoder Jul 18, 2023

nopcoder Jul 18, 2023

N-o-Z Jul 18, 2023 •

edited

Loading

nopcoder Jul 18, 2023

nopcoder Jul 18, 2023

nopcoder Jul 18, 2023

nopcoder Jul 18, 2023

nopcoder left a comment

rmoff left a comment •

edited

Loading

N-o-Z commented Jul 19, 2023 •

edited

Loading

Potential blocker: This change will remove quickstart email capture

Nits

Pre-create the quickstart repo

Sample repo instructions

Startup Banner

rmoff commented Jul 19, 2023

N-o-Z commented Jul 19, 2023

rmoff left a comment

nopcoder Jul 19, 2023

nopcoder Jul 20, 2023

nopcoder Jul 20, 2023

guy-har left a comment

		treeverse/lakefs:latest

		- name: Build and push lakefs quickstart

-	getInstrumentation(metadata)
+	metadata["is_k8s"] = inK8sMetadata()
+	metadata["is_docker"] = inDockerMetadata()
+	metadata["instrumentation"] = instrumentationMetadata()

		accessKey := "AKIAIOSFOLQUICKSTART"
		secretKey := "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"

		fmt.Printf("Access Key ID : %s \n", accessKey)
		fmt.Printf("Secret Access Key: %s \n", secretKey)

	func useConfig(cfg string) bool {
	func useConfig(flagName string) bool {

Add lakefs instrumentation #6217

Add lakefs instrumentation #6217

Conversation

N-o-Z commented Jul 17, 2023

Linked Issue

github-actions bot commented Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmoff left a comment

Choose a reason for hiding this comment

N-o-Z commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

N-o-Z commented Jul 18, 2023

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

N-o-Z Jul 18, 2023 • edited Loading

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

nopcoder left a comment

Choose a reason for hiding this comment

rmoff left a comment • edited Loading

Choose a reason for hiding this comment

🛑 Potential blocker: This change will remove quickstart email capture

Nits

Pre-create the quickstart repo

Sample repo instructions

Startup Banner

N-o-Z commented Jul 19, 2023 • edited Loading

Potential blocker: This change will remove quickstart email capture

Nits

Pre-create the quickstart repo

Sample repo instructions

Startup Banner

rmoff commented Jul 19, 2023

N-o-Z commented Jul 19, 2023

rmoff left a 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

guy-har left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 17, 2023 •

edited

Loading

N-o-Z Jul 18, 2023 •

edited

Loading

rmoff left a comment •

edited

Loading

N-o-Z commented Jul 19, 2023 •

edited

Loading