Skip to content
This repository has been archived by the owner on Jan 22, 2021. It is now read-only.

metrics: add support for more metric types #74

Merged
merged 8 commits into from
Nov 6, 2019

Conversation

akshaysngupta
Copy link
Member

We are leveraging this project to get metrics for an azure resource onto k8s.
Adding support for more metric types to support our use case.

@jsturtevant
Copy link
Collaborator

@akshaysngupta the image build is failing. is make build working locally?

@akshaysngupta
Copy link
Member Author

akshaysngupta commented Oct 29, 2019

no actually, I am not able to go beyond on my machine. It gets stuck here during dep ensure
image

I was able to build the image using docker build -f .\Dockerfile -t <reg>/adapter:latest . and test it on k8s cluster.
image

@jsturtevant
Copy link
Collaborator

dep ensure is hanging because you are missing hg. Run apt-get install mercurial to get it then that should work.

I found the error related to the build issue and working on resolving it. Stay tuned.

@jsturtevant
Copy link
Collaborator

Can you rebase? It will pull in a change that will resolve the failed build.

@jsturtevant
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@jsturtevant
Copy link
Collaborator

@akshaysngupta it looks like the existing Queue Test is not returning the correct values and is getting zero for every request. I am guessing something isn't quite right in the logic to select the type of the Value.

What is the metric and type you are running in your local instance?

@akshaysngupta
Copy link
Member Author

I am using metric of Average Type

@jsturtevant
Copy link
Collaborator

Could you share the ExternalMetric CRD you are using?

@akshaysngupta
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 74 in repo Azure/azure-k8s-metrics-adapter

@jsturtevant
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

Copy link
Collaborator

@jsturtevant jsturtevant 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 contribution! I left a few comments in the code. In addition it would be useful to add an e2e to cover the aggregation type. The easiest way to add this would probably use the already loaded Queue and an additional CRD to check a different aggregation type (maybe Max or Min?)

.env.example Outdated Show resolved Hide resolved
pkg/azure/externalmetrics/monitor_client.go Show resolved Hide resolved
pkg/azure/externalmetrics/monitor_client.go Outdated Show resolved Hide resolved
script/start.sh Outdated Show resolved Hide resolved
@jsturtevant
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@jsturtevant
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

Copy link
Collaborator

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

Really close. Just a few extra checks in case there is bad data that comes back.

pkg/azure/externalmetrics/monitor_client.go Outdated Show resolved Hide resolved
hack/e2e-scripts/gen-and-check-queue-messages.sh Outdated Show resolved Hide resolved
pkg/azure/externalmetrics/monitor_client.go Show resolved Hide resolved
pkg/azure/externalmetrics/monitor_client.go Show resolved Hide resolved
@jsturtevant
Copy link
Collaborator

/azp run

@jsturtevant jsturtevant merged commit 8ddd042 into Azure:master Nov 6, 2019
@akshaysngupta akshaysngupta deleted the aksgupta/metrics branch November 6, 2019 22:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants