Skip to content

/prometheus

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

retrieval: TargetManager and friends need tests. #1906

Closed
beorn7 opened this Issue Aug 22, 2016 · 1 comment

Comments

Assignees
No one assigned
Labels
component/service discovery kind/enhancement
Projects
None yet
Milestone
No milestone
1 participant
@beorn7
@beorn7
Copy link
Member

beorn7 commented Aug 22, 2016

There are no unit tests for the code in targetmanager.go (TargetManager, targetSet, …). In fact, the existing file targetmanager_test.go is empty.

@beorn7 beorn7 added kind/enhancement component/service discovery labels Aug 22, 2016

beorn7 added a commit that referenced this issue Aug 22, 2016

@beorn7
retrieval: Clean up target group map on config reload 
Also, remove unused `providers` field in targetSet.

If the config file changes, we recreate all providers (by calling
`providersFromConfig`) and retrieve all targets anew from the newly
created providers. From that perspective, it cannot harm to clean up
the target group map in the targetSet. Not doing so (as it was the
case so far) keeps stale targets around. This mattered if an existing
key in the target group map was not overwritten in the initial fetch
of all targets from the providers. Examples where that mattered:

```
scrape_configs:
- job_name: "foo"
  static_configs:
  - targets: ["foo:9090"]
  - targets: ["bar:9090"]
```
updated to:
```
scrape_configs:
- job_name: "foo"
  static_configs:
  - targets: ["foo:9090"]
```

`bar:9090` would still be monitored. (The static provider just
enumerates the target groups. If the number of target groups
decreases, the old ones stay around.

```
scrape_configs:
- job_name: "foo"
  dns_sd_configs:
  - names:
    - "srv.name.one.example.org"
```
updated to:
```
scrape_configs:
- job_name: "foo"
  dns_sd_configs:
  - names:
    - "srv.name.two.example.org"
```

Now both SRV records are still monitored. The SRV name is part of the
key in the target group map, thus the new one is just added and the
old ane stays around.

Obviously, this should have tests, and should have tests before, not
only for this case. This is the quick fix. I have created
#1906 to track test
creation.

Fixes #1610 .
Loading status checks…
e2b3626

@beorn7 beorn7 referenced this issue Aug 22, 2016

Merged

retrieval: Clean up target group map on config reload #1907

dmilstein added a commit to dmilstein/prometheus that referenced this issue Aug 26, 2016

@dmilstein
Add basic test for TargetManager.targetSet 
Verify that if the configs change, target groups are cleaned on
TargetManager.reload (rather than having old ones linger around, even if
they are no longer present in the configs).

This covers the bug fixed in prometheus#1907 -- I verified that by checking out
source from before that commit.

This is a start on prometheus#1906
7921601

@dmilstein dmilstein referenced this issue Aug 26, 2016

Merged

Add basic test for TargetManager.targetSet #1926

@beorn7 beorn7 closed this Jan 6, 2017

@lock

This comment has been minimized.

Sign in to view
Copy link

lock bot commented Mar 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.