Skip to content

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented Mar 15, 2015

Move all Pushgateway related top-level functions to push.go.

@brian-brazil @juliusv This fixes #98 and contributes to prometheus/pushgateway#27 .

Move all Pushgateway related top-level functions to push.go.
@brian-brazil
Copy link
Contributor

In the simpleclient, the push a single collector feature is just a convenience function. The core functionality is being able to push a given registry.

If we're adding new functions, instance should not be part of the api - wanting to push an instance label into the push gateway is a extremely rare use case that makes no sense for the vast majority of use cases.

@beorn7
Copy link
Member Author

beorn7 commented Mar 15, 2015

@brian-brazil Once we have user-accessible registries, it will be trivial to push them. (Either there will be a method on it, as it is done internally now, registry.Push(), or it will be the other way round, Push(registry).) In any case, the functions introduced here will be convenience functions. I was tempted to call them that way now, but that would have been confusing, as the registry is not publicly accessible.

WRT to the instance label: We only have them as a special label because after an extensive usage study at SC, they were desired. I cannot take them away now with the "makes no sense" argument.

Should we decide to not add instance labels automatically by the Pushgateway (which I am in favor of, cf. prometheus/pushgateway#18 ), the need for adding instance labels in a convenient way will be even more pressing in the existing use cases.

@brian-brazil
Copy link
Contributor

We only have them as a special label because after an extensive usage study at SC, they were desired. I cannot take them away now with the "makes no sense" argument.

My understanding is that it was in use in SC due to a misunderstanding of the data model, and the textfile collector was what was actually wanted.

The default api needs to not have them as an argument, as it stands users are required to supply instance labels in cases where they explicitly shouldn't have them. As-is this is a badly broken api, and we need to fix it.

@beorn7
Copy link
Member Author

beorn7 commented Mar 15, 2015

Sure, once everybody uses the textfile exporter, we can deprecate the API.

If we want to get rid of the instance focus in the pushgateway, great, but then let's do it properly and not have an inconsistent state where within the same library, some parts support the one semantic and others the other.

@beorn7
Copy link
Member Author

beorn7 commented Mar 15, 2015

To be clear: The functions added here are not so much meant as a new feature, but as a work-around for pushing too many metrics with the default registry. Our users might do it "the wrong way", but I still have to offer them a quick fix. Migration to "the right way" should and will happen in a separate, more relaxed process.

@brian-brazil
Copy link
Contributor

Hmm, 👍 for now.

We'll likely need to move the push functions to a different directory as part of the deprecation (and we should deprecate ASAP).

@beorn7
Copy link
Member Author

beorn7 commented Mar 15, 2015

Yes, agreed. Let's get a clear mind of how things should look on the pushgateway (as the root of all evil :), see prometheus/pushgateway#18 .

beorn7 added a commit that referenced this pull request Mar 15, 2015
Add functions to push individual collectors to a Pushgateway.
@beorn7 beorn7 merged commit 811a3a9 into master Mar 15, 2015
@beorn7 beorn7 deleted the beorn7/push-collector branch March 15, 2015 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add convenience function to push individual collectors.

2 participants