Skip to content

Conversation

@Krukov
Copy link
Contributor

@Krukov Krukov commented Feb 10, 2019

Hello!
Regading #363

We have the same problem with asyncio using (aiohttp, single process, single thread)

As we cannot predict when gc.collect() will be executed (with gc.enable), it is not possible to use something that has locks inside the gc.collect callbacks. So, YES, we neet to reconsidered how the GC collector works.

One way it is remove GC Collector 😎 .
Another way in this PR - Collect metrics in simle structures witout locks and represent it in 'collect' method.
Therd way is to use gc.stats (since python 3.4) with Gauge (GaugeMetricFamily) metrics at collect.
Or as @megabotan said, just remove lock in get methof of MutexValue

Signed-off-by: Krukov Dima <glebov.ru@gmail.com>
Signed-off-by: Krukov Dima <glebov.ru@gmail.com>
Signed-off-by: Krukov Dima <glebov.ru@gmail.com>
Signed-off-by: Krukov Dima <glebov.ru@gmail.com>
Copy link
Contributor

@brian-brazil brian-brazil 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 looking into this.

Switching to gc.stats sounds good to me, we'll only lose 3.3 support.

@brian-brazil
Copy link
Contributor

I still think we'd be better off using gc.stats.

@Krukov
Copy link
Contributor Author

Krukov commented Feb 12, 2019

@brian-brazil Agree with you. I will prepare PR asap

Signed-off-by: Krukov Dima <glebov.ru@gmail.com>
@Krukov
Copy link
Contributor Author

Krukov commented Feb 12, 2019

@brian-brazil I remade PR to using gc.stats. Can you review it?

Signed-off-by: dmitry.kryukov <dmitry.kryukov@exness.com>
@Krukov Krukov changed the title WIP: Deadlock on gcCollector Deadlock on gcCollector Feb 13, 2019
collected = Histogram(
def collect(self):
collected = GaugeMetricFamily(
'python_gc_collected_objects',
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't these all counters?

object_collected and objects_uncollectable would also be more usual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please explain to me:
''object_collected'' Are you taking about variables names or about metrics names?
Is your question about choosing the type of metrics?

Copy link
Contributor

Choose a reason for hiding this comment

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

The metric names.

Also, the types don't seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the metrics names and redid it with counter type

Signed-off-by: dmitry.kryukov <dmitry.kryukov@exness.com>
'python_gc_collected_objects',
def collect(self):
collected = CounterMetricFamily(
'python_gc_objects_collected',
Copy link
Contributor

Choose a reason for hiding this comment

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

Counters should end in _total

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CounterMetricFamily will add it autocratically. You can see it in tests https://github.com/prometheus/client_python/pull/371/files#diff-b8616fecc6de945cdf630d3ad09aebb8R26

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, I'd forgotten I'd already added that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants