Cleanup tron scribereader #1012
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yaroliakh
commented
Dec 16, 2024
Comment on lines
-130
to
-365
| "component": "stderr", | ||
| "message": "line 3", | ||
| "timestamp": "2021-01-02T18:12:09.169421619Z", | ||
| "cluster": "fake" | ||
| }""", | ||
| """{ | ||
| "tron_run_number": 1234, | ||
| "component": "stdout", | ||
| "message": "line 4", | ||
| "timestamp": "2021-01-02T18:12:09.169421619Z", | ||
| "cluster": "bad_fake" | ||
| }""", | ||
| ] | ||
| ) | ||
| output = read_log_stream_for_action_run( | ||
| action_run_id="namespace.job.1234.action", | ||
| component="stdout", | ||
| min_date=min_date, | ||
| max_date=max_date, | ||
| paasta_cluster="fake", | ||
| ) | ||
|
|
||
| mock_stream_reader.assert_called_once_with( | ||
| stream_name="stream_paasta_app_output_namespace_job__action", | ||
| min_date=min_date, | ||
| max_date=max_date, | ||
| reader_host="host", | ||
| reader_port=1234, | ||
| ) | ||
| mock_stream_tailer.assert_called_once_with( | ||
| stream_name="stream_paasta_app_output_namespace_job__action", | ||
| lines=-1, | ||
| tailing_host="host", | ||
| tailing_port=1234, | ||
| ) | ||
| assert output == ["line 0", "line 1", "line 2"] | ||
|
|
||
|
|
||
| def test_read_log_stream_for_action_run_min_date_and_max_date_in_past(): | ||
| # NOTE: these tests don't actually depend on the current time apart from | ||
| # today vs not-today and the args are forwarded to scribereader anyway | ||
| # so using the current time is fine | ||
| min_date = datetime.datetime.now() - datetime.timedelta(days=5) | ||
| max_date = datetime.datetime.now() - datetime.timedelta(days=4) | ||
| with mock.patch( | ||
| "tron.utils.scribereader.get_scribereader_host_and_port", | ||
| autospec=True, | ||
| return_value=("host", 1234), | ||
| ), mock.patch( | ||
| "tron.utils.scribereader.scribereader.get_stream_reader", | ||
| autospec=True, | ||
| ) as mock_stream_reader, mock.patch( | ||
| "tron.utils.scribereader.scribereader.get_stream_tailer", | ||
| autospec=True, | ||
| ) as mock_stream_tailer, mock.patch( | ||
| "tron.utils.scribereader.get_superregion", | ||
| autospec=True, | ||
| return_value="fake", | ||
| ), mock.patch( | ||
| "tron.config.static_config.build_configuration_watcher", | ||
| autospec=True, | ||
| ), mock.patch( | ||
| "staticconf.read", autospec=True, side_effect=static_conf_patch({"logging.max_lines_to_display": 1000}) | ||
| ), mock.patch( | ||
| "tron.config.static_config.load_yaml_file", | ||
| autospec=True, | ||
| ): | ||
| # all the data we want is from the past, so we should only check the reader | ||
| mock_stream_reader.return_value.__enter__.return_value = iter( | ||
| [ | ||
| """{ | ||
| "tron_run_number": 1234, | ||
| "component": "stdout", | ||
| "message": "line 0", | ||
| "timestamp": "2021-01-02T18:10:09.169421619Z", | ||
| "cluster": "fake" | ||
| }""", | ||
| ] | ||
| ) | ||
| # so lets make sure we don't call the tailer | ||
| mock_stream_tailer.return_value.__iter__.side_effect = Exception | ||
| output = read_log_stream_for_action_run( | ||
| action_run_id="namespace.job.1234.action", | ||
| component="stdout", | ||
| min_date=min_date, | ||
| max_date=max_date, | ||
| paasta_cluster="fake", | ||
| ) | ||
|
|
||
| mock_stream_reader.assert_called_once_with( | ||
| stream_name="stream_paasta_app_output_namespace_job__action", | ||
| min_date=min_date, | ||
| max_date=max_date, | ||
| reader_host="host", | ||
| reader_port=1234, | ||
| ) | ||
| mock_stream_tailer.assert_not_called() | ||
| assert output == ["line 0"] | ||
|
|
||
|
|
There was a problem hiding this comment.
those are scribereader specific tests that don't apply to S3LogsReader (there is no difference in whether start and end dates are the same or not)
yaroliakh
commented
Dec 16, 2024
Comment on lines
-2
to
-13
| dateglob==1.1.1 # required by yelp-logging | ||
| geogrid==2.1.0 # required by yelp-logging | ||
| monk==3.0.4 # required by yelp-clog | ||
| ply==3.11 # required by thriftpy2 | ||
| scribereader==1.1.1 # used by tron to get tronjob logs | ||
| simplejson==3.19.2 # required by yelp-logging | ||
| srv-configs==1.3.4 # required by monk | ||
| tenacity==8.2.3 # required by yelp-logging | ||
| thriftpy2==0.4.20 # required by monk | ||
| yelp-cgeom==1.3.1 # required by geogrid | ||
| yelp-clog==7.1.4 # scribereader dependency | ||
| yelp-logging==4.17.0 # scribereader dependency |
There was a problem hiding this comment.
shouldn't be needed anymore with scribereader removal, e.g tested locally with https://fluffy.yelpcorp.com/i/vVZmLRKnVk2SFT3LNvmZljTg8G3WLqcF.html
There was a problem hiding this comment.
ah, actually yelp-clog still pulls a bunch of dependencies that aren't actually used. I'll see if it's possible to make them optional
|
updated a bit the PR to completely replace clog with logreader package. Removed any |
nemacysts
approved these changes
Jan 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Scribereader is not used anymore in Tron. Instead S3LogsReader will be used by default.
Check commits for easier review