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

chore: standardize env for query service #5670

Closed
wants to merge 2 commits into from

Conversation

slayer321
Copy link

Summary

Standardize ENV variable for query service

Related Issues / PR's

fixes #5266

Copy link

welcome bot commented Aug 11, 2024

Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@slayer321
Copy link
Author

@shivanshuraj1333 Can you please review it.

@shivanshuraj1333
Copy link
Member

This is not what I intended,
add something like this, and then do a bootstrap load at query service start, handle errors if there is any access issue.

package config

import (
	"os"
)

type Config struct {
	SignozJwtSecret                               string
	StorageType                                   string
	ClickHouseUrl                                 string
	ClickHouseOptimizeReadInOrderRegex            string
	ClickHouseMaxExecutionTimeLeaf                string
	ClickHouseTimeoutBeforeCheckingExecutionSpeed string
	ClickHouseMaxBytesToRead                      string
	SmtpEnabled                                   string
	DeploymentType                                string
	SmtpHost                                      string
	SmtpPort                                      string
	SmtpUsername                                  string
	SmtpPassword                                  string
	SmtpFrom                                      string
}

var AppConfig Config

func LoadConfig() {
	AppConfig = Config{
		SignozJwtSecret:                               os.Getenv("SIGNOZ_JWT_SECRET"),
		StorageType:                                   os.Getenv("STORAGE"),
		ClickHouseUrl:                                 os.Getenv("CLICKHOUSE_URL"),
		ClickHouseOptimizeReadInOrderRegex:            os.Getenv("CLICKHOUSE_OPTIMIZE_READ_IN_ORDER_REGEX"),
		ClickHouseMaxExecutionTimeLeaf:                os.Getenv("CLICKHOUSE_MAX_EXECUTION_TIME_LEAF"),
		ClickHouseTimeoutBeforeCheckingExecutionSpeed: os.Getenv("CLICKHOUSE_TIMEOUT_BEFORE_CHECKING_EXECUTION_SPEED"),
		ClickHouseMaxBytesToRead:                      os.Getenv("CLICKHOUSE_MAX_BYTES_TO_READ"),
		SmtpEnabled:                                   os.Getenv("SMTP_ENABLED"),
		DeploymentType:                                os.Getenv("DEPLOYMENT_TYPE"),
		SmtpHost:                                      os.Getenv("SMTP_HOST"),
		SmtpPort:                                      os.Getenv("SMTP_PORT"),
		SmtpUsername:                                  os.Getenv("SMTP_USERNAME"),
		SmtpPassword:                                  os.Getenv("SMTP_PASSWORD"),
		SmtpFrom:                                      os.Getenv("SMTP_FROM"),
	}
}

and use it like below
jwtSecret := config.AppConfig.SignozJwtSecret

@slayer321
Copy link
Author

Ok, make sense.
About handle errors part do we want to have something like below ?

 // validateConfig checks if the configuration is valid
func validateConfig(config Config) error {
	if config.SignozJwtSecret == "" {
		return errors.New("missing SIGNOZ_JWT_SECRET")
	}
	if config.StorageType == "" {
		return errors.New("missing STORAGE_TYPE")
	}
	if config.ClickHouseUrl == "" {
		return errors.New("missing CLICKHOUSE_URL")
	}
	if config.SmtpHost == "" {
		return errors.New("missing SMTP_HOST")
	}
	if config.SmtpPort == 0 {
		return errors.New("invalid SMTP_PORT")
	}
	if config.SmtpUsername == "" {
		return errors.New("missing SMTP_USERNAME")
	}
	if config.SmtpPassword == "" {
		return errors.New("missing SMTP_PASSWORD")
	}
	if config.SmtpFrom == "" {
		return errors.New("missing SMTP_FROM")
	}
	return nil
}

@shivanshuraj1333
Copy link
Member

No,
before accessing the flags in the code, handle the error there using existing error handling (mostly log the error).

}
if len(queryServiceConfig.AppConfig.ClickHouseMaxBytesToRead) == 0 {
zap.L().Warn("No ClickHouseMaxBytesToRead env is specified.")
}
Copy link
Author

@slayer321 slayer321 Aug 15, 2024

Choose a reason for hiding this comment

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

I thought of having an helper function which will have this check inside it and log it if env variables not found so that we don't need to repeat the same check

func EnvVariableChecker(env ...string) {
	for _, val := range env {
		if len(val) == 0 {
		  zap.L().Warn(fmt.Sprintf("No %s env is specified.", val))
		}
	}
}

but in this case the %s will be empty as there will be no value. So added the check for all the env variables.

@Aeolun
Copy link

Aeolun commented Oct 4, 2024

Shouldn't this check and alert all errors on load? I don't think the system is much use without a ClickhouseServer variable defined.

@prashant-shahi prashant-shahi requested a review from a team December 19, 2024 12:21
@prashant-shahi prashant-shahi changed the base branch from develop to main December 19, 2024 12:21
@grandwizard28
Copy link
Collaborator

Hi @slayer321,
Thank you for the PR! We are however in the middle of a major codebase revamp where we will also be standardising config as per #6805.

As part of the config being created, we are also going to validate and fail fast. That should take care of @Aeolun's point!

@grandwizard28 grandwizard28 added the duplicate This issue or pull request already exists label Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardize ENV for query service
5 participants