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

Move main logic into a sub-package for easier re-use #5

Closed
wants to merge 2 commits into from

Conversation

cgilling
Copy link
Contributor

This pull request satisfies #3.

One thing that I'd like some other peoples' opinion on is the interface for creating and configuring a Collector. Currently there is a Config struct that is passed into the New constructor, and a DefaultConfig that defines a Config pre-populated with useful defaults. This was done mainly so that the bool config values could default to true rather than false.

Ideally I would like to have something more like this:

type Collector struct {
    Addr      string
    Prefix    string
    PauseDur  time.Duration
    EnableCPU bool
    EnableMem bool
    EnableGC  bool
    Done      <-chan struct{}

    s      g2s.Statter
}

func (c *Collector) Run() error {
    // create client and return error if there is one
    // setup key of it is default value
    ....
}

The problem here is that I would have to change the Enable* to Disable* which seems not ideal as we would need double negatives in the code if !c.DisableCPU {. But perhaps it is the best way to do it to make the interface easier to understand and closer to what is done in the standard library (like http.Server)

Once we've decided what we want the interface to be I will change it to be that way and add in comment on the exported types as well as a package comment on go-runtime-metrics/collector.

Looking at the differences between the vendored version and the
upstream from peterbourgon/g2s there seems to be no changes that
are actually used by this package. Seems best to use the cannonical
package unless there are good reasons.
@cgilling
Copy link
Contributor Author

going with solution that is now implemented in #6

@cgilling cgilling closed this Aug 26, 2015
@cgilling cgilling deleted the librarize branch September 8, 2015 01:12
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.

None yet

1 participant