pkg / profile

Simple profiling for Go
BSD 2-Clause "Simplified" License
1.99k stars 124 forks source link

Named interface for Stop() #60

Open kishaningithub opened 3 years ago

kishaningithub commented 3 years ago

Problem

Currently the signature of the Start function is

func Start(options ...func(*Profile)) interface {
    Stop()
} 

Use case

The lifecycle in our case is managed by another library (fluent-bit-go)

Proposal

have a named interface for Stop

type Stopper interface {
    Stop()
} 

So the Start will become

func Start(options ...func(*Profile)) Stopper
davecheney commented 3 years ago

Thank you for raising this issue. Before we jump to solutions, can you explain why this is a problem?

kishaningithub commented 3 years ago

Thanks for the quick response @davecheney

It is mainly because the Start() and Stop() has to be executed in 2 different places for fluent bit go lang plugins

To give a bit more context, Go plugin for fluent bit has the following lifecycle

Register -> Initialize -> Flush -> Exit

The flush gets called multiple times (~million times)

Initialize and exit gets called only once

I want the Start() to happen in initialize and Stop to happen in Exit()

Eg - https://github.com/fluent/fluent-bit-go/blob/master/examples/out_multiinstance/out.go#L21

Why is this a problem ?

I would like to assign the return value of the Start() to a variable and store it in a struct. During the Exit() i would access the value of this struct and call the Stop() method

Currently i am doing like the following

type PluginContext struct {
   ProfileStopper interface {
    Stop()
  } 
}

You can see a duplication of the Stop() interface in my code(the consumer of this library)

I would like to avoid this duplication and declare it just like this

type PluginContext struct {
   ProfileStopper profile.Stopper
}
davecheney commented 3 years ago

As far as I understand this should work as the method sets of the anonymous and named interface as the same

https://play.golang.org/p/UVPn8HHZ-37

kishaningithub commented 3 years ago

Thanks for the example @davecheney

The problem is not that it does not work, the problem is there is no named interface as far as i can see inside this library because of which i have to copy paste the same interface definition in my code

davecheney commented 3 years ago

Could you show me what you’ve tried? Maybe we can fix it?

kishaningithub commented 3 years ago

Currently the code i implemented is same as your example. There is no problem, it works

I felt it would be better if the anonymous interface has a name then i wont have to duplicate the interface definition as a consumer.

davecheney commented 3 years ago

I felt it would be better if the anonymous interface has a name then i wont have to duplicate the interface definition as a consumer.

No, that's the wrong way to write Go. The caller should declare the interface it expects, don't import a package simply for its interface declaration.

kishaningithub commented 3 years ago

That's a good thought @davecheney Thanks for sharing!

Just so that i understand clearly, Does this mean

kishaningithub commented 3 years ago

If you can point me to some articles/blogs that's fine too 😊

kishaningithub commented 3 years ago

I still dont see this as a bad idea though. For example take this issue

https://github.com/golang/go/issues/35306

Go lang wants to create a named interface for the Unwrap(), As(), Is() methods