util/httputil: use const string instead of variable int.

[johncming]

It's better to use const string than variable int.

Because pathParam is only visible in a package, it's not a bug but an improvement.

[roidelapluie]

Thanks for the pull request.

regarding int -> var: what is the added value? why is it better?

can we use a const int? with zero value?
In this context it all belogs to this file, the name of the variable is mort important than the content.

[johncming]

Thanks for the pull request.

regarding int -> var: what is the added value? why is it better?

can we use a const int? with zero value?
In this context it all belogs to this file, the name of the variable is mort important than the content.

The code is good which has no bugs. Mainly for better understanding.

The better reason for using const is that the context.Value here need an unchanged key.
The better reason for using const string is that a string is meaningful than a zero number. Anyway, it has to initialize const value.

Thanks.

[bwplotka]

LGTM

[roidelapluie]

int has a smaller footprint. What matters here is the name of the variable for the visibility. Adding a value equal to the variable name does not really add value IMO, even for readability.

const is an improvement however ; and it can be done with a one liner.

WDYT?

[johncming]

int has a smaller footprint. What matters here is the name of the variable for the visibility. Adding a value equal to the variable name does not really add value IMO, even for readability.

const is an improvement however ; and it can be done with a one liner.

WDYT?

It's hard to say that int is cheaper than string.

go version go1.13.8 darwin/amd64

goos: darwin
goarch: amd64
pkg: tmp
BenchmarkIntVarKey-8   	1000000000	         0.867 ns/op	       0 B/op	       0 allocs/op
BenchmarkIntKey-8      	1000000000	         0.472 ns/op	       0 B/op	       0 allocs/op
BenchmarkStringKey-8   	1000000000	         0.447 ns/op	       0 B/op	       0 allocs/op

The test cases:

package do_test

import (
	"context"
	"testing"
)

type (
	Int    int
	String string
)

var (
	intVarKey Int = 0
)

const (
	intKey    Int    = 0
	stringKey String = "test"
)

func BenchmarkIntVarKey(b *testing.B) {
	b.ReportAllocs()
	ctx := context.Background()
	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {
			ctx.Value(intVarKey)
		}
	})
}

func BenchmarkIntKey(b *testing.B) {
	b.ReportAllocs()
	ctx := context.Background()
	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {
			ctx.Value(intKey)
		}
	})
}

func BenchmarkStringKey(b *testing.B) {
	b.ReportAllocs()
	ctx := context.Background()
	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {
			ctx.Value(stringKey)
		}
	})
}

[bwplotka]

I believe int vs string discussion on some tiny const used as Context Key is definitely a micro optimization, so let's not waste our time on this (: Whatever is more readable here, happy with both string or int. (: 👍

[roidelapluie]

Suggested change

	const (
	pathParam ctxParam = "path_param"
	)
	const pathParam ctxParam = "path"

[bwplotka]

👍

[roidelapluie]

I'd suggest moving to a one liner with shorter value.

[johncming]

updated.

[roidelapluie]

While you are at it:

can you add type queryCtx int in promql/engine.go in this PR ?

[johncming]

While you are at it:

can you add type queryCtx int in promql/engine.go in this PR ?

👍 updated.

[roidelapluie]

Suggested change

	const queryOrigin queryCtx = "query"
	const queryOrigin queryCtx = "origin"

makes more sense

[beorn7]

https://golang.org/pkg/context/#WithValue recommends struct{}. I find string confusing as it suggests the value of the key has any meaning.

[beorn7]

I tried out an alternative approach, see #6965.

[beorn7]

(I had it in code anyway to play with it, and it was easier to just push it instead of explaining it here.)

[roidelapluie]

@johncming do you agree with #6965 ?

[johncming]

@johncming do you agree with #6965 ?

Some discussions in golang:

golang/go#33742
golang/go#17826

There are some nice optimizations to prevent certain interface allocations, but they shouldn't be documented here. Best to stick with simple rules of thumb.

It looks like an old discussion. 😊 There are more than one best practices.

[roidelapluie]

You did not answer my question.

[beorn7]

Even besides the allocation discussion, I think the struct{} idiom nicely illustrates that it's only about the (named) type, not the value assigned to it. Using a string as the key would totally make sense if we had different keys of the same type and used the string content to distinguish. But we are only using the type here, with the assigned value being irrelevant. That's why I find struct{} as the underlying type better than int. You could still assign a value to a variable of a named type with int as the underlying type, while that's not even possible with struct{}.

[johncming]

You did not answer my question.

struct{} is also a best practice. It's in the standard library files, after all. As I mentioned above, there are a number of best practices.

const string and const int are also best practices that have no allocation which just likes it says in the go-doc. The main reason for the reduced allocation here is the recursive call when calling Value.

I think all three are OK.

[johncming]

Even besides the allocation discussion, I think the struct{} idiom nicely illustrates that it's only about the (named) type, not the value assigned to it. Using a string as the key would totally make sense if we had different keys of the same type and used the string content to distinguish. But we are only using the type here, with the assigned value being irrelevant. That's why I find struct{} as the underlying type better than int. You could still assign a value to a variable of a named type with int as the underlying type, while that's not even possible with struct{}.

I feel that all three options are subjective. From a view of binding variable in a functional language, it looks more like const usage which looks more natural.

[beorn7]

I think if you assign a value to a variable, const or not, and you never inspect it, that's objectively weird.

But I also don't feel super strongly about it. Just wanted to suggest something that IMHO makes most sense in this case and is also shorter. Whatever you others decide…

[roidelapluie]

Going for @beorn7 as there is less repetition in the names etc. It is, I think, a bit more elegant.

Thank you for your work!