Multi platform support

[hakobe]

This is experiment branch to support multi platform.

• Some refactoring to separate metrics/spec generators
• Separates metrics/spec generators for linux to independent packages

[mattn]

SGTM

[hakobe]

Thanks @mattn! You've been a great help to me.

I'll fix some points from you, and this PR will be merged after the reviewing by an another hatena staff.

[motemen]

The PluginGenerator may have configuration key for logging/debugging purpose (someday)

[hakobe]

That's so true.. It'll be necessary someday.

[motemen]

LGTM

[hakobe]

This branch will be merged after field tests on hatena's servers, so that this is the first time to use build constraints.

[mattn]

Cool

[mattn]

One thing, time.Duration is a type of what you see. So you shouldn't set 1 like https://github.com/mackerelio/mackerel-agent/blob/multi-platform/command/command_linux.go#L25
It should be &FooGenerator{1 * time.Duration}. And use directly like time.Sleep(foo.Interval).
Or change the type to int.

Ooops sorry.

[mattn]

https://github.com/mackerelio/mackerel-agent/blob/multi-platform/command/command_linux.go#L25

I'm thinking some platforms are require to store some member variables to keep condition while collecting values. So it's better to make functions like NewFooGenerator() to hide initialization of them.

For example, on linux,

&FooGenerator{0}

But on windows, possible to be

&FooGenerator{0, 0, false}

NewFooGenerator() can hide the differences.

[hakobe]

@mattn. I think it is not necessary to create functions like NewFooGenerator()

We are planning to create platform specific functions which creates generator structs like below.

// in command/command_freebsd.go
// ...
func specGenerators() []spec.Generator {
    return []spec.Generator{
        &specFreeBSD.KernelGenerator{"some", "field", "for", "FreeBSD"},
        &specFreeBSD.CPUGenerator{"other", "fields" },
        /* ... */
    }
}
// ...

This is no problem, because specGenerators() is only required to return a slice of Generators. The way to initialize Generators does not matter.

I think that differences of the Generator initialization among architectures are already hidden by specGenerators() and metricsGenerators().

[mattn]

Sure. This way has one more point. Even though it will need to add new field to the struct in the future, we don't need to change command/command_xxx.go :)

[mattn]

However, it's not important or critical things. So go on your way! :)

[hakobe]

Yes, I agree that initializing structs by functions like NewHogefuga is better way to increase maintainability. Regardless of architecture problems, we should create such functions.

As you say it's not important or critical things still now. We will solve this problem by another Pull Request. Thank you for your advice !:smile:

[mattn]

日本語ですみません。

後でこれに対するWindowsパッチを書きますが(というかほぼ出来てます)、Windowsの場合は構造体にスナップショットを取った際の状態(Windowsのパフォーマンスカウンタのハンドル)を保持しています。そのハンドルの初期化処理を行う必要があり、Generate でさせるとなると初期化済みフラグみたいな物を持つ必要があります。

できれば command/command_xxx.go にはWindows API呼出しなどは入れたくないので、おそらくWindowsに限ってはNewFooGenerator(1)形式にさせて頂く事になると思います。

[hakobe]

僕も日本語がいいかと思ってました！

なるほどなるほど。たしかにアーキテクチャによっては初期化関数がないと実装が難しいということはありそうです。

Windows版に関しては、関数を使って初期化していただくので問題ないかと思います。specGenerators()/metricsGenerators()がGeneratorの列を返却する限りは、ほかの部分に影響はないと思っています。

(とはいえ、コードのスタイルがアーキテクチャごとに違うのも良くないので、Linux部分についても関数で初期化するスタイルにおいおい揃えたほうが良い、ということにはなりそうですね。)

Windows版については、このブランチのコードでも、まだアーキテクチャ依存の箇所があると思うので(pidとかPOSIXパスになっている部分とか)、構造を変えたほうが良さそうな部分があれば教えていただけると助かります。metrics/specに関する部分以外では、必要最低限の部分だけ、アーキテクチャごとにコードを分けることで、なるべくコードの重複がないようにしたいと思っています。

[hakobe]

はてなでのサーバでの動作検証ができましたので、マージいたします。

[mattn]

👍