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/vcenter] Fixes multiple datacenters issue #33735

Merged

Conversation

StefanKurek
Copy link
Contributor

Description:
Fixes issue with client when there are multiple datacenters. Now first all datacenters (object type) will be collected. They will be passed into the problematic client methods. These methods will now set each datacenter in the list individually as the internal client finds the relevant objects and adds to a master list.

Link to tracking Issue:
#33734

Testing:
Unit test tweaked to find issue. Failure noted. Then passing after fix was added.

Documentation:
None needed.

.chloggen/vcenterreceiver_fix_multiple_datacenters.yaml Outdated Show resolved Hide resolved
.chloggen/vcenterreceiver_fix_multiple_datacenters.yaml Outdated Show resolved Hide resolved
cmd/otelcontribcol/go.mod Outdated Show resolved Hide resolved
@StefanKurek StefanKurek force-pushed the fix/vcenter-multiple-datacenters branch from 588a3cc to f5f74ae Compare June 28, 2024 16:46
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Just some minor wording nits, otherwise LGTM.

component: vcenterreceiver

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Fixes errors when multiple datacenters in some of the client calls.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
note: Fixes errors when multiple datacenters in some of the client calls.
note: Fixes errors in some of the client calls for environments containing multiple datacenters.

Minor wording nit, the current wording of when multiple datacenters is unclear (at least to me).

@@ -128,13 +130,30 @@ func (v *vcenterMetricScraper) scrapeAndProcessAllMetrics(ctx context.Context, e
return errs.Combine()
}

// scrapeDatacenterInventoryListObjects scrapes and store all Datacenter objects with their InventoryLists
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// scrapeDatacenterInventoryListObjects scrapes and store all Datacenter objects with their InventoryLists
// scrapeDatacenterInventoryListObjects scrapes and stores all Datacenter objects with their InventoryLists

@StefanKurek StefanKurek force-pushed the fix/vcenter-multiple-datacenters branch from 0980475 to e2b7278 Compare June 28, 2024 23:03
@djaglowski djaglowski merged commit 88e18f7 into open-telemetry:main Jul 1, 2024
154 checks passed
@StefanKurek StefanKurek deleted the fix/vcenter-multiple-datacenters branch July 1, 2024 12:48
@github-actions github-actions bot added this to the next release milestone Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants