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

Add support for Logs config API #270

Merged
merged 6 commits into from
Sep 13, 2019

Conversation

tt810
Copy link
Contributor

@tt810 tt810 commented Sep 11, 2019

Support for Logs public config API pipeline and pipelinelist. Pipelinelist has read and update functions. When a 'pipeline' is created, it's automatically included in the pipelinelist, thus there is no creation or deletion for pipelinelist.

logs_processor.go Outdated Show resolved Hide resolved

// convert converts the first argument of type interface{} to a map of string and interface{}
// and sign the result to the second argument.
func convert(definition interface{}, processor map[string]interface{}) error {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is semi generic, semi specific, e.g. we have generic types with specific argument names, can we make it fully generic or fully specific please ? Also why do we need to marshal unmarshal and not just cast ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree that the 3 steps (json marshal/unmarshal and all marshal) seems heavy. Isn't there another way to manage it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I will change the method name to be more specific.
  2. Also why do we need to marshal unmarshal and not just cast? -> A simple cast will create an extra layer for Definition which does not exist in JSON format from config API. There are a few ways to do this step, this is the most concise one. As in terms of efficiency, the extra step of conversion is negligible compare to the API calls. After discussing with @bkabrda, I decide to go with the way "marshal/unmarshal and marshal again".

Copy link
Contributor Author

@tt810 tt810 Sep 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. I retried the cast in an explicit method:

func buildProcessor(definition interface{}) map[string]interface{} {
	return definition.(map[string]interface{})
}

MarshalJSON() {
...
case ArithmeticProcessorType:
		mapProcessor = buildProcessor(processor.Definition.(ArithmeticProcessor))
}

It doesn't complain. But I was doing the same thing directly in the MarshalJSON method:
Screen Shot 2019-09-12 at 11 16 51
I can't assert a specific type to a general type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment of this method to explain why we need to do this extra converting. General reference: https://stackoverflow.com/questions/23589564/function-for-converting-a-struct-to-map-in-golang/42849112#42849112

Copy link
Contributor Author

@tt810 tt810 Sep 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step back to see other approaches for the struct of Pipeline and Processor. We can:
define

type LogsPipeline struct {
	Id         *string              `json:"id,omitempty"`
	...
	Processors []map[string]interface{}      `json:"processors,omitempty"`
}

Or

type LogsPipeline struct {
	Id         *string              `json:"id,omitempty"`
	...
	Processors []interface{}      `json:"processors,omitempty"`
}

Both will lose the types/structures for processors. It omits the custom serialization/deserialization part (also means I might need to take care of special cases (e.g. nil/null conversion). But it adds weaker conversion on provider side(resource <-> struct). I don't see it better than the current version besides no need to do the marshal/unmarshal/marshal for Definition.
Another thing is current supported resources have their own types (widget in dashboard has similar definition). The current approach is compatible with the rest of resources.

logs_pipeline_list.go Outdated Show resolved Hide resolved
logs_pipeline.go Outdated Show resolved Hide resolved
logs_pipeline.go Outdated Show resolved Hide resolved
logs_processor.go Outdated Show resolved Hide resolved

// convert converts the first argument of type interface{} to a map of string and interface{}
// and sign the result to the second argument.
func convert(definition interface{}, processor map[string]interface{}) error {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree that the 3 steps (json marshal/unmarshal and all marshal) seems heavy. Isn't there another way to manage it?

@tt810 tt810 force-pushed the ting.tu/logs-config-api branch from 5d6eabe to 7ef7a66 Compare September 12, 2019 09:36
Copy link
Collaborator

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks awesome, thanks for submitting it! I posted some questions/comments inline and I did thumbsup on some already existing comments so that some minor fixes are done. But overall this code looks really nice, so we should be able to merge it with only couple minor adjustments that were requested.

logs_pipeline.go Outdated Show resolved Hide resolved
logs_processor.go Outdated Show resolved Hide resolved
@tt810 tt810 force-pushed the ting.tu/logs-config-api branch from 7ef7a66 to fa31ebb Compare September 12, 2019 14:00
@tt810 tt810 force-pushed the ting.tu/logs-config-api branch from fa31ebb to edd7c8f Compare September 12, 2019 14:07
Copy link
Collaborator

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, merging. Thanks for the awesome PR!

@bkabrda bkabrda merged commit eefe521 into zorkian:master Sep 13, 2019
@tt810 tt810 deleted the ting.tu/logs-config-api branch September 13, 2019 15:05
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.

4 participants