Incorrect version in greptime_app_version

[zyy17]

What type of bug is this?

Other

What subsystems are affected?

Other

Minimal reproduce step

Access the /metrics endpoint when the DB is ready:

bash-5.1# curl -q  http://10.244.1.27:4000/metrics | grep greptime_app
greptime_app_version app version
# TYPE greptime_app_version gauge
greptime_app_version{short_version="0.9.1",version="-fe1cfbf2"} 1

The version should be fe1cfbf2, not -fe1cfbf2.

What did you expect to see?

We should get the correct version without the prefix -.

What did you see instead?

Get the version with prefix -.

What operating system did you use?

Linux

What version of GreptimeDB did you use?

v0.9.1

Relevant log output and stack trace

No response

[leaf-potato]

Maybe I can give it a try.

[zyy17]

@leaf-potato it's yours. Have fun. Feel free to ask if anything needs to be clarified.

[leaf-potato]

The version uses the branch-commit format. When git checkout to a commit, there is no branch.

greptimedb/src/common/version/src/lib.rs

Lines 127 to 129 in 93be81c

	pub const fn short_version() -> &'static str {
	const_format::formatcp!("{}-{}", BUILD_INFO.branch, BUILD_INFO.commit_short,)
	}

Therefore, fix it by using commit id directly when the branch is empty.

[zyy17]

The version uses the branch-commit format. When git checkout to a commit, there is no branch.

greptimedb/src/common/version/src/lib.rs

Lines 127 to 129 in 93be81c

	pub const fn short_version() -> &'static str {
	const_format::formatcp!("{}-{}", BUILD_INFO.branch, BUILD_INFO.commit_short,)
	}

Therefore, fix it by using commit id directly when the branch is empty.

I'm confused with short_version(CARGO_PKG_VERSION) and version(${commit_short}-${branch}). In fact, version is some kind of "short" version.

Can we just return branch and commit_short? For example:

pub const fn branch_and_short_commit() -> (&'static str, &'static str) {
    (BUILD_INFO.branch, BUILD_INFO.commit_short)
} 

IMO, version should be semantic, for example, v0.9.1. We can make greptime_app_version looks like:

greptime_app_version{version="0.9.1",short_commit="fe1cfbf2",branch="main"}

cc @MichaelScofield @tisonkun

[leaf-potato]

I'm confused with short_version(CARGO_PKG_VERSION) and version(${commit_short}-${branch}). In fact, version is some kind of "short" version.

Mabey it's a bug. When calling the log_version function, pass the return value of the short_version() function to the app_version parameter:

greptimedb/src/cmd/src/flownode.rs

Lines 218 to 220 in 2c3fccb

	log_versions(version(), short_version());
	info!("Flownode start command: {:#?}", self);

However, inside the function log_version, the value of app_version is assigned to version instead of short_version:

greptimedb/src/cmd/src/lib.rs

Lines 31 to 34 in 93be81c

	lazy_static::lazy_static! {
	static ref APP_VERSION: prometheus::IntGaugeVec =
	prometheus::register_int_gauge_vec!("greptime_app_version", "app version", &["short_version", "version"]).unwrap();
	}

greptimedb/src/cmd/src/lib.rs

Lines 79 to 86 in 93be81c

	pub fn log_versions(version_string: &str, app_version: &str) {
	// Report app version as gauge.
	APP_VERSION
	.with_label_values(&[env!("CARGO_PKG_VERSION"), app_version])
	.inc();
	// Log version and argument flags.
	info!("GreptimeDB version: {}", version_string);

The simple solution is to swap the current assignment operation.

[MichaelScofield]

@leaf-potato It's unusual that "branch" is empty. I was digging into the "shadow-rs"(that find the git info) one time, and suspect the incorrectness was origin from its usage of "git2". Maybe you can disable the "git2" feature and give it a try?

[zyy17]

I'm confused with short_version(CARGO_PKG_VERSION) and version(${commit_short}-${branch}). In fact, version is some kind of "short" version.

Mabey it's a bug. When calling the log_version function, pass the return value of the short_version() function to the app_version parameter:

greptimedb/src/cmd/src/flownode.rs

Lines 218 to 220 in 2c3fccb

	log_versions(version(), short_version());
	info!("Flownode start command: {:#?}", self);

However, inside the function log_version, the value of app_version is assigned to version instead of short_version:

greptimedb/src/cmd/src/lib.rs

Lines 31 to 34 in 93be81c

	lazy_static::lazy_static! {
	static ref APP_VERSION: prometheus::IntGaugeVec =
	prometheus::register_int_gauge_vec!("greptime_app_version", "app version", &["short_version", "version"]).unwrap();
	}

greptimedb/src/cmd/src/lib.rs

Lines 79 to 86 in 93be81c

	pub fn log_versions(version_string: &str, app_version: &str) {
	// Report app version as gauge.
	APP_VERSION
	.with_label_values(&[env!("CARGO_PKG_VERSION"), app_version])
	.inc();
	// Log version and argument flags.
	info!("GreptimeDB version: {}", version_string);

The simple solution is to swap the current assignment operation.

Two version labels still confuse the user. The user has to understand(or we assume the user knows) the format of app_version, which is constructed with branch and commit. It will be clearer if we use the raw git info as the metric label, for example, branch and commit.

[evenyag]

Two version labels still confuse the user. The user has to understand(or we assume the user knows) the format of app_version, which is constructed with branch and commit. It will be clearer if we use the raw git info as the metric label, for example, branch and commit.

+1

[leaf-potato]

and suspect the incorrectness was origin from its usage of "git2"

@MichaelScofield Maybe not. Disabling git2 in shadow-rs also has the same problem:

#before:
shadow-rs = "0.31"

#after:
shadow-rs = { version = "0.31", default-features = false}

It's unusual that "branch" is empty.

You can reproduce the problem by following the steps below:

git checkout HEAD^
git branch --show-current

[leaf-potato]

It will be clearer if we use the raw git info as the metric label, for example, branch and commit

Got it!

[MichaelScofield]

@leaf-potato If you do git checkout like that, it's in the "detached" mode, there's no branch. A walkaround might be git branch --remote --contains | sed "s|[[:space:]]*origin/||"

[leaf-potato]

git branch --remote --contains | sed "s|[[:space:]]*origin/||"

You also need to filter out HEAD: git branch --remote --contains | sed "s|[[:space:]]*origin/||"|grep -v "HEAD". Should this be supported by shadow-rs?

[MichaelScofield]

@leaf-potato Wait...On second thought, do we really need the "branch" info? "commit id" is sufficient to uniquely locate the codes revision. The way to find branch in detached mode is kind of magic, you can try to fire an issue in shardow-rs.

[baoyachi]

When building GitLab or GitHub, it will check out from a certain branch, causing the command to retrieve the current branch with empty content.

The delivered product (binary) version should be concrete, and in fact, it is more recommended to use the git tag mechanism for tagging, because branches are often deleted, and git tags are usually treated as archives.

When building shadow-rs, it is recommended to use git tag recording in this way. For details, please refer to.

If you want to choose between branch or tag, you need to make a judgment. The pseudocode is roughly as follows:

{
 "version":shadow::PKG_VERSION,
+ "branch/tag":if ! shadow::BRANCH.is_empty(){ shadow::BRANCH} else{shadow::LAST_TAG},
"commit_hash":shadow::SHORT_COMMIT,
"build_time":shadow::BUILD_TIME,
}