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

[MRG+1] Show elapsed time in statscollector #3638

Merged
merged 5 commits into from Apr 6, 2019
Merged

Conversation

maramsumanth
Copy link
Contributor

Showing elapsed time in seconds in stats, also changing from utc to local time for better user experience.
(In my opinion time&date should be displayed based on the local time)

@codecov
Copy link

codecov bot commented Feb 24, 2019

Codecov Report

Merging #3638 into master will increase coverage by 0.2%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #3638     +/-   ##
=========================================
+ Coverage   84.52%   84.73%   +0.2%     
=========================================
  Files         167      168      +1     
  Lines        9410     9464     +54     
  Branches     1397     1407     +10     
=========================================
+ Hits         7954     8019     +65     
+ Misses       1199     1188     -11     
  Partials      257      257
Impacted Files Coverage Δ
scrapy/extensions/corestats.py 90% <100%> (+1.53%) ⬆️
scrapy/http/__init__.py 100% <0%> (ø) ⬆️
scrapy/linkextractors/__init__.py 100% <0%> (ø) ⬆️
scrapy/downloadermiddlewares/retry.py 95.83% <0%> (ø) ⬆️
scrapy/linkextractors/lxmlhtml.py 92.4% <0%> (ø) ⬆️
scrapy/linkextractors/sgml.py 96.8% <0%> (ø) ⬆️
scrapy/http/request/json_request.py 93.75% <0%> (ø)
scrapy/settings/default_settings.py 98.64% <0%> (ø) ⬆️
scrapy/item.py 98.48% <0%> (+0.07%) ⬆️
scrapy/crawler.py 92.39% <0%> (+0.32%) ⬆️
... and 1 more

@maramsumanth
Copy link
Contributor Author

@lopuhin , can you look into this?

Copy link
Member

@lopuhin lopuhin left a comment

Choose a reason for hiding this comment

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

Hey @maramsumanth adding elapsed_time_seconds looks like a useful addition to me, would be interesting to hear what others think.
Added some comments regarding the implementation. Also I think we need to have tests for the new feature, and update the docs.

scrapy/extensions/corestats.py Outdated Show resolved Hide resolved
scrapy/extensions/corestats.py Outdated Show resolved Hide resolved
@maramsumanth
Copy link
Contributor Author

Hey @kmike @whalebot-helmsman , what do you think of adding this feature?

self.stats.set_value('finish_time', datetime.datetime.utcnow(), spider=spider)
finish_time = datetime.datetime.utcnow()
elapsed_time = finish_time - self.stats.get_value('start_time')
elapsed_time_seconds = elapsed_time.seconds + elapsed_time.microseconds/1000000
Copy link
Member

Choose a reason for hiding this comment

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

In Python 2 elapsed_time.microseconds/1000000 is always 0

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 think this is failing for python 2.7, what should I modify then @kmike ?

@kmike
Copy link
Member

kmike commented Mar 18, 2019

Hey @maramsumanth! I think having elapsed_time_seconds in stats is a good feature.

It'd be good to add a test for this feature (e.g. modify test_closespider_timeout - which is incorrect, by the way, as it adds seconds and microseconds)

@maramsumanth
Copy link
Contributor Author

@kmike, can you please review this. Should I change close_on , since it is failing for version 2.7

scrapy/extensions/corestats.py Outdated Show resolved Hide resolved
Co-Authored-By: maramsumanth <maram.sumanth@gmail.com>
@maramsumanth
Copy link
Contributor Author

Thanks @Gallaecio :)

@Gallaecio Gallaecio changed the title Show elapsed time in statscollector [MRG+1] Show elapsed time in statscollector Mar 23, 2019
@kmike
Copy link
Member

kmike commented Apr 6, 2019

Thanks @maramsumanth and @Gallaecio!

@kmike kmike merged commit aa46e19 into scrapy:master Apr 6, 2019
@kmike kmike added this to the v1.7 milestone Jun 25, 2019
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.

None yet

4 participants