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

[receiver/mongodbatlas] Refactored to use New metric builder #9093

Merged

Conversation

cpheps
Copy link
Contributor

@cpheps cpheps commented Apr 5, 2022

Description: Used latest version of mdatagen on mongodbatlas receiver and updated scraper code to utilize generated structures while trying to leave the majority of code intact where possible.

Not all metric and resource attributes where captures in the metadata.yaml so I updated that. I also renamed some resource attributes so they are all prefixed with mongodb_atlas where before only some where.

Link to tracking Issue:
Resolves #7145
Resolves #8831

Testing: Current tests still pass. Manually compared results against previous results before change and matches according (minus some resource property name changes)

Documentation: Generated metric docs have changed.

@cpheps cpheps requested a review from a team as a code owner April 5, 2022 17:53
@cpheps cpheps requested a review from dmitryax April 5, 2022 17:53
@project-bot project-bot bot added this to In progress in Collector Apr 5, 2022
@cpheps
Copy link
Contributor Author

cpheps commented Apr 5, 2022

@zenmoto Would you mind giving a review since you are codeowner?

@zenmoto
Copy link
Contributor

zenmoto commented Apr 6, 2022

That is a dramatic improvement.

@cpheps cpheps force-pushed the mongodbatlas-new-metric-builder branch from a37a455 to 8753c7f Compare April 6, 2022 12:05
@cpheps cpheps force-pushed the mongodbatlas-new-metric-builder branch from 8753c7f to 7510181 Compare April 6, 2022 19:22
@cpheps cpheps requested a review from dmitryax April 7, 2022 12:10
Corbin Phelps added 4 commits April 8, 2022 11:30
Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
@cpheps cpheps force-pushed the mongodbatlas-new-metric-builder branch from 7510181 to 44d0f50 Compare April 8, 2022 15:31
@cpheps
Copy link
Contributor Author

cpheps commented Apr 8, 2022

@dmitryax Can I get a re-review on this please?

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Sorry for the late response


return &m, nil
func MeasurementsToMetric(mb *MetricsBuilder, meas *mongodbatlas.Measurements, buildUnrecognized bool) error {
recordFunc := getRecordFunc(meas.Name)
Copy link
Member

Choose a reason for hiding this comment

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

My idea for the switch case was to avoid passing functions. Something like this:

for _, point := range meas.DataPoints {
    if point.Value != nil {
        curTime, err := time.Parse(time.RFC3339, point.Timestamp)
        if err != nil {
            return err
        }
        record(mb, meas, point, curTime)
    }
}

And inside record we use switch case to make particular call.

Not needed for this PR tho. Can be done separately.

Collector automation moved this from In progress to Reviewer approved Apr 8, 2022
@djaglowski djaglowski merged commit 439369e into open-telemetry:main Apr 9, 2022
@cpheps cpheps deleted the mongodbatlas-new-metric-builder branch April 11, 2022 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Reviewer approved
Development

Successfully merging this pull request may close these issues.

[receiver/mongodbatlas] Duplicate Metrics Migrate mongodbatlasreceiver to the new Metrics Builder
4 participants