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

[Datadog] Add support for Logs Public config API #312

Merged
merged 12 commits into from
Sep 25, 2019
Merged

Conversation

tt810
Copy link
Contributor

@tt810 tt810 commented Sep 11, 2019

Supporting logs Public config API pipeline and pipeline order. for more information please read this

@ghost ghost added the size/XXL label Sep 11, 2019
Copy link

@ajacquemot ajacquemot left a comment

Choose a reason for hiding this comment

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

Left a bunch of comments/questions, did not read at the unit-tests yet.

datadog/resource_datadog_logs_pipeline.go Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Outdated Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Outdated Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Outdated Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Outdated Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Outdated Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Outdated Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Outdated Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Outdated Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Outdated Show resolved Hide resolved
Copy link

@ajacquemot ajacquemot left a comment

Choose a reason for hiding this comment

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

Looks nice, left few comments.

"strings"
)

const (

Choose a reason for hiding this comment

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

Can we group remappers and processors please ? It should ease readability

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 was sorting them alphabetically. It should be easier for the people who are not familiar with logs backend related logic? In addition, they are all called processors.

Choose a reason for hiding this comment

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

I don’t think so, it just grouping by suffix basically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const (
	tfArithmeticProcessor        = "arithmetic_processor"
	tfAttributeRemapperProcessor = "attribute_remapper"
	tfCategoryProcessor          = "category_processor"
	tfDateRemapperProcessor      = "date_remapper"
	tfGrokParserProcessor        = "grok_parser"
	tfMessageRemapperProcessor   = "message_remapper"
	tfNestedPipelineProcessor    = "pipeline"
	tfServiceRemapperProcessor   = "service_remapper"
	tfStatusRemapperProcessor    = "status_remapper"
	tfTraceIdRemapperProcessor   = "trace_id_remapper"
	tfUrlParserProcessor         = "url_parser"
	tfUserAgentParserProcessor   = "user_agent_parser"
)

To me the order is alphabetic:

tfArithmeticProcessor        
tfAttributeRemapperProcessor 
tfCategoryProcessor          
tfDateRemapperProcessor      
tfGrokParserProcessor        
tfMessageRemapperProcessor   
tfNestedPipelineProcessor    
tfServiceRemapperProcessor   
tfStatusRemapperProcessor    
tfTraceIdRemapperProcessor   
tfUrlParserProcessor         
tfUserAgentParserProcessor

tfUserAgentParserProcessor = "user_agent_parser"
)

var tfProcessorTypes = map[string]string{

Choose a reason for hiding this comment

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

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above.

tfUserAgentParserProcessor: datadog.UserAgentParserType,
}

var ddProcessorTypes = map[string]string{

Choose a reason for hiding this comment

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

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above.

datadog/resource_datadog_logs_pipeline.go Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Show resolved Hide resolved
Copy link
Contributor

@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.

Hi, thanks so much for this PR, great work! I left several comments inline that should be addressed.

datadog/provider.go Outdated Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Show resolved Hide resolved
datadog/resource_datadog_logs_pipelineorder.go Outdated Show resolved Hide resolved
datadog/resource_datadog_logs_pipelineorder.go Outdated Show resolved Hide resolved
website/datadog.erb Outdated Show resolved Hide resolved
website/datadog.erb Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
datadog/import_datadog_logs_pipeline_test.go Show resolved Hide resolved
Copy link
Contributor

@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.

I mistakenly approved the PR in my previous review and I can't change the status of the review, so I'm adding an additional empty review with status "Request changes".

datadog/resource_datadog_logs_pipeline.go Show resolved Hide resolved
datadog/resource_datadog_logs_pipeline.go Outdated Show resolved Hide resolved
page_title: "Datadog: datadog_logs_pipeline"
sidebar_current: "docs-datadog-resource-logs-pipeline"
description: |-
Provides a Datadog logs pipeline resource. This can be used to create and manage logs pipelines.

Choose a reason for hiding this comment

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

Nit: Logs pipelines & processors

Copy link
Contributor Author

@tt810 tt810 Sep 20, 2019

Choose a reason for hiding this comment

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

Why do we need to explicitly state processors? Shouldn't it be a property of pipeline? From the current documentation for public config API, we don't have explicit endpoints to access processors.


# datadog_logs_pipeline

Provides a Datadog [Logs Pipeline API](https://docs.datadoghq.com/api/?lang=python#logs-pipelines) resource. This can be used to create and manage Datadog logs pipelines.

Choose a reason for hiding this comment

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

Same as above

website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

Docs review

website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline_order.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline_order.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline_order.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline_order.html.markdown Outdated Show resolved Hide resolved
website/docs/r/logs_pipeline_order.html.markdown Outdated Show resolved Hide resolved
@tt810 tt810 requested a review from bkabrda September 23, 2019 08:09
Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

👍 for docs

@tt810
Copy link
Contributor Author

tt810 commented Sep 25, 2019

Hi @ruthnaebeck, I added a recommendation at the end of website/docs/r/logs_pipeline.html.markdown could you check again please?


const (
tfArithmeticProcessor = "arithmetic_processor"
tfAttributeRemapperProcessor = "attribute_remapper"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the processor suffix for a remapper? It is a TF convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is processor is a general name which describes the processor properties of pipeline. Either a remapper or something else, they are all processors. I want to make it clear that they are all processors in TF.

Copy link

@ajacquemot ajacquemot left a comment

Choose a reason for hiding this comment

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

Thanks for this work, this looks good to me 🚀

There are still some open questions left, please answer all of them before merging.

Copy link
Contributor

@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.

Allright folks, I think all the questions/comments have been addressed or replied to and I see no more issues preventing me from merging this PR.

Big thanks to everyone who added their reviews and huge thanks to @tt810 for implementing all of this functionality!

Merging 🎉

@bkabrda bkabrda merged commit fab5cb0 into DataDog:master Sep 25, 2019
@ruthnaebeck
Copy link
Contributor

Hi @ruthnaebeck, I added a recommendation at the end of website/docs/r/logs_pipeline.html.markdown could you check again please?

Looks good :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants