Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

LaterGauge needs to call thread safe functions #3722

Merged
merged 5 commits into from
Aug 21, 2018

Conversation

erikjohnston
Copy link
Member

The prometheus collector can be run on a separate thread, and so the functions that the LaterGauge call should be thread safe. This PR fixes the two worst offenders, but there are still a bunch of other gauges who are doing a bit of naughty things

@erikjohnston erikjohnston requested a review from a team August 20, 2018 16:42
@@ -111,6 +112,9 @@
# The set of all in flight requests, set[RequestMetrics]
_in_flight_requests = set()

# Protects the _in_flight_requests set from concurrent accesss
_in_flight_reqeuests_lock = threading.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

requeuests?

for desc, processes in six.iteritems(_background_processes):
# We copy the dict so that it doesn't change from underneath us
with _bg_metrics_lock:
_background_processes_copy = dict(_background_processes)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like dict(_background_processes) shouldn't require a lock?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm completely failing to find any documentation about what is and isn't safe TBH

Copy link
Contributor

@hawkowl hawkowl left a comment

Choose a reason for hiding this comment

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

Seems fine, apart from the typo in request_metrics.py.

@erikjohnston erikjohnston merged commit 2b1a4b2 into develop Aug 21, 2018
@erikjohnston erikjohnston deleted the erikj/bg_process_iteration branch September 20, 2018 13:58
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