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

Cleanup flags prior to 2.0 release #2634

Closed
brian-brazil opened this Issue Apr 18, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@brian-brazil
Copy link
Member

brian-brazil commented Apr 18, 2017

There's various old/deprecated flags we should get rid of (e.g. old alertmanagers flag). This issue is to remind us to do it.

@brian-brazil brian-brazil added this to the v2.x milestone Apr 18, 2017

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jun 15, 2017

This seems mostly done. We have just a handful of flags left.

   -version false
   -config.file "prometheus.yml"
   -alertmanager.notification-queue-capacity 10000
   -alertmanager.timeout 10s
   -log.format "\"logger:stderr\""
   -log.level "\"info\""
   -query.max-concurrency 20
   -query.staleness-delta 5m0s
   -query.timeout 2m0s
   -storage.local.engine "persisted"     <<< remove?
   -storage.local.path "data"                 <<< mv storage.tsdb.path
   -storage.tsdb.max-block-duration 0s
   -storage.tsdb.min-block-duration 2h0m0s
   -storage.tsdb.no-lockfile false
   -storage.tsdb.retention 360h0m0s
   -web.console.libraries "console_libraries"
   -web.console.templates "consoles"
   -web.enable-remote-shutdown false
   -web.external-url
   -web.listen-address ":9090"
   -web.max-connections 512
   -web.read-timeout 30s
   -web.route-prefix
   -web.telemetry-path "/metrics"         <<< drop this? 
   -web.user-assets

overwriting the /metrics path seems pointless. We don't allow randomly rewriting anything else, so what's the reason to do it here? The route prefix allows for all practical cases I assume.

@brian-brazil

This comment has been minimized.

Copy link
Member Author

brian-brazil commented Jun 15, 2017

overwriting the /metrics path seems pointless.

Many of our exporters permit it, for reasons that aren't really clear to me.

-query.staleness-delta 5m0s

Pretty sure this isn't used in 2.0.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jun 15, 2017

So should we ditch /metrics flag for 2.0?

Staleness delta is still used in plenty of cases as our window for how far we look back for instant queries mostly. Even with staleness solved, this needs to be specified somehow. But is the name still apropriate.

@brian-brazil

This comment has been minimized.

Copy link
Member Author

brian-brazil commented Jun 15, 2017

So should we ditch /metrics flag for 2.0?

I'd be in favour, however we'd need to get consensus on removing it from all the other components too.

Staleness delta is still used in plenty of cases as our window for how far we look back for instant queries mostly.

We don't use the flag value in 2.0, we use the constant StalenessDelta.

@gouthamve

This comment has been minimized.

Copy link
Member

gouthamve commented Jun 15, 2017

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jun 15, 2017

I'd be in favour, however we'd need to get consensus on removing it from all the other components too.

@SuperQ @grobie @discordianfish you are dealing a lot with exporters doing this. Opinions?

@SuperQ

This comment has been minimized.

Copy link
Member

SuperQ commented Jun 15, 2017

Yes, we should remove /metrics flag from our components. I don't see a need to provide an override for this in our own code. The only use case is for reverse proxies, but these can be adjusted by users.

@grobie

This comment has been minimized.

Copy link
Member

grobie commented Jun 15, 2017

@brian-brazil

This comment has been minimized.

Copy link
Member Author

brian-brazil commented Jun 15, 2017

From the requests I've seen it seems to be users trying to be consistent with some of their applications that use a (single) non-standard path.

@gouthamve gouthamve referenced this issue Jun 15, 2017

Merged

Move CLI commander to cobra #2844

3 of 3 tasks complete
@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jun 27, 2017

This is done.

@fabxc fabxc closed this Jun 27, 2017

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 23, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 23, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.