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.

Sign up for GitHub

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

Jump to bottom

Datafactory #242

Merged
merged 1 commit into from
Feb 22, 2021
Merged

Datafactory #242

merged 1 commit into from
Feb 22, 2021

Conversation

nicolasvion
Copy link
Contributor

adding some datafactory detectors

Shr3ps
Shr3ps requested changes Jan 25, 2021
View reviewed changes
modules/integration_azure-datafactory/common-locals.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/common-variables.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/common-versions.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/detectors-datafactory.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/detectors-datafactory.tf Outdated Show resolved Hide resolved
docs/severity.md Outdated

|Detector|Critical|Major|Minor|Warning|Info|
|---|---|---|---|---|---|
|Azure DataFactory Activity Rate Status|X|X|-|-|-|
Copy link
Member

Choose a reason for hiding this comment

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

No heartbeat detector for this resource?

xp-1000
xp-1000 suggested changes Jan 25, 2021
View reviewed changes
Copy link
Contributor

@xp-1000 xp-1000 left a comment

Choose a reason for hiding this comment

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

most of the formula seem not correct or at least there is a useless signal in them.

I see you define Critical / Major everywhere, are you sure all detectors here are all equally important ? (and require a critical severity?).

Also you use lasting everywhere, is there really necessary / wanted ?

modules/integration_azure-datafactory/common-locals.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/common-variables.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/common-versions.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/detectors-datafactory.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/detectors-datafactory.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/detectors-datafactory.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/detectors-datafactory.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/detectors-datafactory.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/detectors-datafactory.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/variables.tf Outdated Show resolved Hide resolved
@BzSpi
Copy link
Contributor

BzSpi commented Jan 25, 2021

Can you please the detectors generation from yaml files as its easier to read and to maintain ?

BzSpi
BzSpi previously requested changes Jan 28, 2021
View reviewed changes
Copy link
Contributor

@BzSpi BzSpi left a comment

Choose a reason for hiding this comment

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

We can also add a detector that checks the resources count against the max since metrics are available.

modules/integration_azure-datafactory/conf/01-adf-activity-run.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/01-adf-activity-run.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/01-adf-activity-run.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/02-adf-pipeline-run.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/01-adf-activity-run.yaml Show resolved Hide resolved
modules/integration_azure-datafactory/conf/04-adf-integration-available-memory.yaml Outdated
name: "adf integration available memory"
filtering: "filter('resource_type', 'Microsoft.Storage/factories') and filter('primary_aggregation_type', 'true')"
aggregation: ".sum(by=['azure_resource_name', 'azure_resource_group_name', 'azure_region'])"
transformation: ".min(over='5m')"
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems quite short since low memory is not critical for this use case.

modules/integration_azure-datafactory/common-modules.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/01-adf-activity-run.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/02-adf-pipeline-run.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/03-adf-trigger-run.yaml Outdated Show resolved Hide resolved
Shr3ps
Shr3ps reviewed Feb 3, 2021
View reviewed changes
common/module/modules.tf Outdated Show resolved Hide resolved
@nicolasvion nicolasvion force-pushed the datafactory branch 2 times, most recently from 5637a87 to 1ac407a Compare February 9, 2021 14:32
Shr3ps
Shr3ps requested changes Feb 11, 2021
View reviewed changes
modules/integration_azure-datafactory/conf/01-adf-activity-run.yaml Outdated
module: "Azure DataFactory"
name: "activity error rate"
filtering: "filter('resource_type', 'Microsoft.DataFactory/factories') and filter('primary_aggregation_type', 'true')"
aggregation: ".sum(by=['name'])"
Copy link
Member

Choose a reason for hiding this comment

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

hum, is this enough? what if you have multiple pipeline with the same name il multiple rg/regions?

@nicolasvion nicolasvion force-pushed the datafactory branch 2 times, most recently from 6248542 to 9df06c2 Compare February 11, 2021 16:30
xp-1000
xp-1000 suggested changes Feb 17, 2021
View reviewed changes
Copy link
Contributor

@xp-1000 xp-1000 left a comment

Choose a reason for hiding this comment

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

overall seems good for me, see comments for requested changes including:

  • useless rollup to delete
  • undesirable changes out of scope (e.g. main readme)
  • rename modules file

also I doubt that all percentage based detectors have the same combination of thresholds and severity but I cannot say it is up to you.

README.md Outdated Show resolved Hide resolved
common/module/modules.tf Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/02-adf-pipeline-run.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/02-adf-pipeline-run.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/03-adf-trigger-run.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/04-adf-integration-available-memory.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/05-adf-integration-cpu-percentage.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/05-adf-integration-cpu-percentage.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/conf/05-adf-integration-cpu-percentage.yaml Outdated Show resolved Hide resolved
modules/integration_azure-datafactory/common-modules.tf Outdated Show resolved Hide resolved
@nicolasvion nicolasvion force-pushed the datafactory branch 4 times, most recently from 82e7fe6 to 27d41cf Compare February 17, 2021 16:27
xp-1000
xp-1000 approved these changes Feb 17, 2021
View reviewed changes
Copy link
Contributor

@xp-1000 xp-1000 left a comment

Choose a reason for hiding this comment

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

looks good for me thanks

@xp-1000 xp-1000 requested review from Shr3ps and BzSpi February 17, 2021 16:33
@xp-1000
add datafactory module
9d94b1d 
- adding datafactory v1.0
- adding rg and region filter, and some docs
- updating detectors for datafactory part
@xp-1000 xp-1000 merged commit 8dc8732 into master Feb 22, 2021
@xp-1000 xp-1000 deleted the datafactory branch February 22, 2021 18:42
@xp-1000 xp-1000 added this to the v1.2.0 milestone Feb 22, 2021
@xp-1000 xp-1000 added this to In progress in Claranet via automation Feb 22, 2021
@xp-1000 xp-1000 added detectors About nex or existing detectors new feature Request for new feature labels Feb 22, 2021
@xp-1000 xp-1000 self-assigned this Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shr3ps Shr3ps Shr3ps approved these changes

@xp-1000 xp-1000 xp-1000 approved these changes

@cvauvarin cvauvarin Awaiting requested review from cvauvarin cvauvarin is a code owner

@BzSpi BzSpi Awaiting requested review from BzSpi BzSpi is a code owner

Assignees

@xp-1000 xp-1000

Labels
detectors About nex or existing detectors new feature Request for new feature
Projects
Claranet
In progress
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nicolasvion @BzSpi @Shr3ps @xp-1000