Make /metrics endpoint configurable

[tus]

Make /metrics endpoint configurable

[Acconut]

Or perhaps, if we're going down this path anyway, maybe we can make it apparent we are not talking about local paths, and also be more specific than base

I don't fully understand, because we are talking about local paths. It's the paths on which tusd will accept the tus requests. Whether these come through a proxy or not does not matter for it.

[kvz]

If you consider the promdash example https://github.com/prometheus/promdash#deploy-behind-a-reverse-proxy, they allow a web.external-url parameter, giving complete control over the Location being returned to the client. If you'd wish to preserve the original host and such and not care, that is still possible by just specifying /metrics or /files/ - nothing changes. But if you wish to route traffic some place else (maybe a specific region, or maybe restore port/host information that was tempered with due some layers of proxies) that would also be possible by means of the endpoint/url argument, rather than a path argument.

For instance, maybe a request hits uploads.megacorp.com but you're using local file storage only, having deployed failover pairs of uploaders, and want to make sure the next call hits their specific hostname, then one could say:

tusd \
  -files-endpoint https://uploader001.megacorp.com/resumable/files/ \
  -metrics-endpoint https://uploader001.megacorp.com/resumable/metrics

[Acconut]

But if you wish to route traffic some place else (maybe a specific region, or maybe restore port/host information that was tempered with due some layers of proxies)

This is what the -behind-proxy flag is for. It tells tusd to read the protocol, hostname and port from the Forwarded and X-Forwarded-* headers.

[kvz]

we are talking about local paths

Well, I feel -base-path has a higher probability for being mistaken for -dir than -files-endpoint would have. So I mean local paths in that sense: local directories. The endpoint terminology registers more with URLs in my brain at least.

This is what the -behind-proxy flag is for. It tells tusd to read the protocol, hostname and port from the Forwarded and X-Forwarded-* headers.

That is a great feature. It does not however give the person controlling tusd (but not controlling the proxy) the same flexibility as the endpoint argument would, it does for instance not cover the use-case I mentioned in my previous reply.

But I get the feeling you like -base-path much better?

[Acconut]

Well, I feel -base-path has a higher probability for being mistaken for -dir than -files-endpoint would have.

True, but that's what descriptions are for (I know, the current ones could be better).

But I get the feeling you like base-path much better?

Not necessary, but I do not like changing the current one since this is a breaking change. And I guess, we both know why these are bad, especially since Go package usages are not versioned.

For instance, maybe a request hits uploads.megacorp.com but you're using local file storage only, having deployed failover pairs of uploaders, and want to make sure the next call hits their specific hostname

We can make -base-path accept absolute URLs

[kvz]

True, but that's what descriptions for (I know, the current ones could be better).

Documentation aside, would you not rather have the most clearest argument names possible to start with?

Not necessary, but I do not like changing the current one since this is a breaking change. And I guess, we both knot why these are bad, especially since Go package usages are not versioned.

This makes sense. However, does this mean we can never push out a breaking version? People using Go are aware and will be mindful when they upgrade, no?

We can make -base-path accept absolute URLs

That's going to seem 'wrong', putting a URL in a path variable. I would not mind pushing out a major release over this. In this case, we can also change the -dir to -store-dir, and make it so that expose-metrics is implied by -metrics-endpoint being true or a string. If metrics-endpoint is false, this indicates no metrics are exposed. It could default to /metrics.

I feel this is still an okay time to clean up our CLI 'API', we will only be less likely to address these issues later on, meaning in four years we'll still have these less than ideal arguments in tusd.

[Acconut]

Documentation aside, would you not rather have the most clearest argument names possible to start with?

Sure, I would like to.

However, does this mean we can never push out a breaking version? People using Go are aware and will be mindful when they upgrade, no?

You can introduce breaking changes in major releases but it's not as easy as with NPM since Go packages are, by itself, not versioned. There is no dominant package manager and quite a lot of people who always pull the latest version from the master branch. Recently, dependency vendoring has been introduced to solve a few issues but it's far from being used wildly enough.

I would not mind pushing out a major release over this.

Frankly, I do not like releasing a major version just because a few flags gets renamed.

make it so that expose-metrics is implied by -metrics-endpoint being true or a string.

I absolutely disagree. expose-metrics is a boolean whereas metrics-endpoint is a string. Two different flags, with different meaning and values, and types. I have seen this too often as a source of confusion and bugs to be willing to make such a change by myself.

[kvz]

Sure, I would like to.

💯

You can introduce breaking changes in major releases but it's not as easy as with NPM since Go packages are, by itself, not versioned. There is no dominant package manager and quite a lot of people who always pull the latest version from the master branch. Recently, dependency vendoring has been introduced to solve a few issues but it's far from being used wildly enough.

In these stages, I feel tusd is mostly used as a tagged binary release, more so than an imported package. People using Go are aware of this trait also. Otherwise no one would ever release a new major ever.

Frankly, I do not like releasing a major version just because a few flags gets renamed.

We should not get sentimental with our versioning, nor should we block progress based on those sentiments. This is not the protocol, where I agree major version bumps are somewhat critical.

I absolutely disagree. expose-metrics is a boolean whereas metrics-endpoint is a string. Two different flags, with different meaning and values, and types.

Anything taken from the command-line is a string, wether you intend an int or boolean, and so those need to be casted to something else anyway. If Flags is getting in your way, how about using "" for a falsey value? All I know is that it would be dreadful to have two arguments for endpoints. One for turning it on, and the other for its value.

I have seen this too often as a source of confusion and bugs to be willing to make such a change by myself.

It's hard to argue your experience without the examples. But I can offer some counter examples we could argue about such as in Docker where there's an argument such as --add-host="". Now it's not adding an empty string and newline to the /etc/hosts file every time you type docker run. Empty string means don't do it. Same could hold true for --metrics-endpoint="".

[Acconut]

We should not get sentimental with our versioning, nor should we block progress based on those sentiments.

It's not that I dislike the look of high version numbers but a major release usually means abandoning the previous as I don't see anyone of us being ready to backport the bug fixes.

Anything taken from the command-line is a string, wether you intend an int or boolean, and so those need to be casted to something else anyway.

Honestly, I do not consider this a good argument. Yes, arguments may be passed to the process in a string format but I don't think this is a reason why a flag cannot have a type. For example, the Golang flag parser does not accept every value for a boolean flag:

$ go run cmd/tusd/main.go -expose-metrics=foo
invalid boolean value "foo" for -expose-metrics: strconv.ParseBool: parsing "foo": invalid syntax

One for turning it on, and the other for its value.

Now imagine a situation where you also want to allow the user to specify the format of which the metrics are output. Would you still try to put this into the one flag or be in this case ready to add an additional flag? I would do latter.

Empty string means don't do it. Same could hold true for --metrics-endpoint=""

You talked about descriptive and clear argument names before. I don't want to be provocative but which line is easier to understand?

tusd -expose-metrics=false
tusd -metrics-endpoint=""

[kvz]

You talked about descriptive and clear argument names before. I don't want to provocative but which line is easier to understand?

To be fair, the comparison would have to be:

tusd -expose-metrics=true
tusd -metrics-endpoint="/metrics"

vs

tusd -metrics-endpoint="/metrics"

In which, to answer you question, I think the second one is easier to understand. I would assume setting this endpoint would expose metrics.

For turning it off, one would just run tusd in both cases, since the default would be "" (or "/metrics" and false in your case).

As for the versioning, I understand backporting fixes would suck. FWIW I don't think we would have to maintain the earlier version just yet. But you also could consider adding backwards compatible flags to the new version, perhaps adding deprecation warnings at one point. In this case, you would not have to bump a major (is tusd even SemVer?)

[Acconut]

I believe this entire conversation boils down to two major topics:

Implicit vs Explicit?

To pick up your last example: tusd -expose-metrics=true -metrics-endpoint="/metrics". I don't mind writing this. Yes, is may be long and it could be shorter but it's explicit. There is not magic happening behind the curtains of which I am not aware of. Just by looking at the line, I can see what is going on and I think this is very precious.

How much will a major release hurt us?

Sadly, we don't have any numbers and must make assumptions. There will be people unhappy about a breaking change and some may benefit from it, we just have to face this and keep it in mind.

But you also could consider adding backwards compatible flags to the new version, perhaps adding deprecation warnings at one point.

This is an acceptable approach. Adding deprecations until we have accumulated enough breaking changes to ratify a major release.

is tusd even SemVer?

Yes, I try to follow it.

[kvz]

To pick up your last example: tusd -expose-metrics=true -metrics-endpoint="/metrics". I don't mind writing this. Yes, is may be long and it could be shorter but it's explicit. There is not magic happening behind the curtains of which I am not aware of. Just by looking at the line, I can see what is going on and I think this is very precious.

Right, but if a user types -metrics-endpoint="/metrics" and forgets to also enable -expose-metrics=true, they will be confused. Why would someone explicitly set a metrics endpoint if they did not want metrics exposed? That's no magic, that's logic, and making the user think/work harder than need be. Sure, rftm and all, but I think we should make an effort to save people roundtrips to these the docs/usage help.

This is an acceptable approach. Adding deprecations until we have accumulated enough breaking changes to ratify a major release.

👍

[Acconut]

I apologize for my belated reply, I lost this conversation from my radar.

Right, but if a user types -metrics-endpoint="/metrics" and forgets to also enable -expose-metrics=true, they will be confused.

This will not happen. The expose-metrics flag is enabled by default, so if only the metrics-endpoint flag is provided, the binary will behave as expected. Here are some examples:

# Both, provide metrics at the /stats endpoint
tusd --enable-metrics --metrics-endpoint="/stats"
tusd --metrics-endpoint="/stats"

# Both, will not provide any metrics but the command displays this in an explicit way, so no confusion occurs.
tusd --enable-metrics=false
tusd --enable-metrics=false --metrics-endpoint="/stats"

[Acconut]

Even though we didn't reach a consensus here, I think we can close this issue. You can disable the metrics endpoint or change it's path, so the original topic of this issue has long been addressed.