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

New resources for Log analytics datasources #8589

Closed
wants to merge 19 commits into from
Closed

New resources for Log analytics datasources #8589

wants to merge 19 commits into from

Conversation

PriyankaRanganath
Copy link
Contributor

@PriyankaRanganath PriyankaRanganath commented Sep 23, 2020

Add support for Log analytics workspace data source resources:

  1. azurerm_log_analytics_datasource_iis_logs
  2. azurerm_log_analytics_datasource_linux_performance_collection
  3. azurerm_log_analytics_datasource_linux_performance_object
  4. azurerm_log_analytics_datasource_linux_syslog_collection
  5. azurerm_log_analytics_datasource_linux_syslog

fixes #3182

@PriyankaRanganath PriyankaRanganath marked this pull request as draft September 23, 2020 07:50
@PriyankaRanganath PriyankaRanganath marked this pull request as ready for review September 23, 2020 12:40
@PriyankaRanganath PriyankaRanganath changed the title Added Log analytics datasource resource features New resources for Log analytics datasource resources Sep 23, 2020
@PriyankaRanganath PriyankaRanganath changed the title New resources for Log analytics datasource resources New resources for Log analytics datasources Sep 23, 2020
@PriyankaRanganath PriyankaRanganath marked this pull request as draft September 24, 2020 10:56
@PriyankaRanganath PriyankaRanganath marked this pull request as ready for review September 25, 2020 05:59
@PriyankaRanganath
Copy link
Contributor Author

PriyankaRanganath commented Sep 25, 2020

Hi @katbyte / @tombuildsstuff ,

Is there any way to review and merge this PR? I understand that there is no official documentation for log analytics data sources properties from Microsoft. However, we have taken reference from the equivalent PowerShell cmdlets for each of these data sources and implemented accordingly. The community can use this meanwhile until official documentation and supporting go SDK is available. There is already implementation for windows event and windows performance counter data sources. I dont see why this PR would be an issue if everything is implemented properly.

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@PriyankaRanganath, thank you so much for opening this PR, this mostly LGTM, however I left a few comments to address. Once those are taken care of we can get this merged! 🚀

@PriyankaRanganath
Copy link
Contributor Author

PriyankaRanganath commented Oct 8, 2020

Hi @WodansSon ,

Thank you for reviewing the PR! Resolved comments as requested

@jackofallops jackofallops modified the milestones: v2.31.0, v2.32.0 Oct 8, 2020
Copy link
Collaborator

@katbyte katbyte 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 the PR @PriyankaRanganath, while overall this looks great it appears the SDK/swagger is incomplete and needs to be updated and unfortunately we need the SDK to define these types else they can change on Azure's side without correctly handling semantic versioning - as such these types need to be present in the Azure SDK/Swagger. There is an open issue here and until this is fixed i'm not sure we can merge this

Comment on lines +69 to +71
type dataSourceIISLogsProperty struct {
State string `json:"state"`
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks like an SDK/swagger bug? could we get an issue opened and this fixed there?


if v, ok := d.GetOk("on_premise_enabled"); ok {
if onPremiseEnabled := v.(bool); onPremiseEnabled {
onPremiseState = "OnPremiseEnabled"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be exposed by the SDK as a constant? which probably ties in to the swagger bug for the missing dataSourceIISLogsProperty struct?

Comment on lines +69 to +71
type dataSourceLinuxPerformanceCollectionProperty struct {
State string `json:"state"`
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment above: could we get this fixed in the swagger and SDK?

Comment on lines +81 to +87
perfCollectionState := "Disabled"

if v, ok := d.GetOk("enabled"); ok {
if enabled := v.(bool); enabled {
perfCollectionState = "Enabled"
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @katbyte , there is no support for SDK or even official documentation from Microsoft yet. Is it ok to still merge this PR ?
I am trying to get hold of Microsoft prod team but as mentioned there is no ETA on it yet

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please see the linked issue below for more details, but we typically refrain from merging anything that is not currently supported by the SDK as it can become problematic down the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @katbyte ! will wait for SDK and official documentation release

Comment on lines +87 to +96
type dataSourceLinuxPerformanceObjectProperty struct {
ObjectName string `json:"objectName"`
InstanceName string `json:"instanceName"`
IntervalSeconds int `json:"intervalSeconds"`
PerformanceCounters []dataSourceLinuxPerformanceObjectCounterName `json:"performanceCounters"`
}

type dataSourceLinuxPerformanceObjectCounterName struct {
CounterName string `json:"counterName"`
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should also be in the swagger & sdk

Comment on lines +69 to +71
type dataSourceLinuxSysLogCollectionProperty struct {
State string `json:"state"`
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

and this?

Comment on lines +106 to +113
type dataSourceLinuxSysLogProperty struct {
SysLogName string `json:"syslogName"`
SysLogSeverities []dataSourceLinuxSysLogSeverity `json:"syslogSeverities"`
}

type dataSourceLinuxSysLogSeverity struct {
Severity string `json:"severity"`
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should also be added to the SDK/swagger

@katbyte
Copy link
Collaborator

katbyte commented Oct 8, 2020

linked issue: Azure/azure-rest-api-specs#9072

@jackofallops jackofallops modified the milestones: v2.32.0, v2.33.0 Oct 15, 2020
@tombuildsstuff tombuildsstuff modified the milestones: v2.33.0, Blocked Oct 21, 2020
@katbyte
Copy link
Collaborator

katbyte commented Oct 22, 2020

@PriyankaRanganath - given this is blocked on the swagger issue, i am going to close it. Once the service team has addressed the missing swagger & SDK supports these properties we can reopen this PR and move it forward.

@katbyte katbyte closed this Oct 22, 2020
@ghost
Copy link

ghost commented Nov 22, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure Data Sources in a Log Analytics Workspace
6 participants