-
Notifications
You must be signed in to change notification settings - Fork 28
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
Stats #159
Stats #159
Conversation
+1 to add a prefix. The main question is should we include page object name or not, i.e. should we aggregate stats across page objects. As the page object may include its name to the stat name if needed, I think a good default is not to include page object name to the prefix, just use "scrapy-poet" or "scrapy_poet" as a prefix (what's the common practice?) - or maybe "scrapy-poet/stats" (if we plan to add out-of-the-box stats to scrapy-poet) As a complete overkill solution, we can have a setting with the stats prefix, and template support, so that user can change the prefix, include/exclude page object name if needed, or even move the stats to the top level. It can even be a list of string templates, so that you can have both per-page-object stats and aggregate stats. Not sure we need to do it though, a single fixed prefix looks perfectly fine. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #159 +/- ##
==========================================
+ Coverage 85.38% 85.62% +0.23%
==========================================
Files 14 14
Lines 787 800 +13
==========================================
+ Hits 672 685 +13
Misses 115 115
|
I’ll undraft as soon as the (internal) -/_ battle for the prefix is over. |
===== | ||
|
||
scrapy-poet tracks :external+web-poet:ref:`web-poet stats <stats>` as part of | ||
:ref:`Scrapy stats <topics-stats>`, prefixed with ``poet/stats/``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last thing to check: let's see how it looks like with "standard stats", like the number of dependencies provided:
"poet/stats/my-custom-1": 15,
"poet/stats/my-custom-2": 15,
"poet/dependencies/MyPageObject": 15,
"poet/dependencies/HttpResponse": 15,
Seems fine to me, so +1 to merge
Follow-up to scrapinghub/web-poet#175.
I wonder if we should include some prefix on stats, like
scrapy_poet/
,scrapy_poet/<page object import path>/
, or ,scrapy_poet/<page object import path>/stats/
.