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

ISSUE-2269: tests for access log transmission to Clickhouse server #749

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

symstu-tempesta
Copy link
Contributor

No description provided.

Copy link
Contributor

@RomanBelozerov RomanBelozerov left a comment

Choose a reason for hiding this comment

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

Please update README.md and setup.sh. Could you add clickhouse installation there.

helpers/clickhouse.py Outdated Show resolved Hide resolved
t_clickhouse/test_clickhouse_logs.py Outdated Show resolved Hide resolved
t_clickhouse/test_clickhouse_logs.py Outdated Show resolved Hide resolved
t_clickhouse/test_clickhouse_logs.py Outdated Show resolved Hide resolved
t_clickhouse/test_clickhouse_logs.py Outdated Show resolved Hide resolved
t_clickhouse/test_clickhouse_logs.py Outdated Show resolved Hide resolved
self.assertEqual(wrk_total_requests, dmesg_logs_count)

def test_all_logs_with_reload(self):
client = self.prepare_client()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you should move these stress tests to t_stress directory? Or you can just add access_log to exists tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean under " add access_log to exists tests" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have some stress tests in t_stress directory. You can add access_log directive to these tests. I don't think that we need a new stress tests.

t_clickhouse/test_clickhouse_logs.py Outdated Show resolved Hide resolved
t_clickhouse/test_clickhouse_logs.py Outdated Show resolved Hide resolved
t_clickhouse/test_clickhouse_logs.py Outdated Show resolved Hide resolved
tests_config.ini.sample Outdated Show resolved Hide resolved
)

record = self.loggers.clickhouse.access_log_last_message()
self.assertEqual(record.status, 500)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes I receive error here:

======================================================================
ERROR: test_clickhouse_record_data_with_post (t_clickhouse.test_clickhouse_logs.TestClickHouseLogsCorrectnessDataPostRequest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/tempesta-test/t_clickhouse/test_clickhouse_logs.py", line 283, in test_clickhouse_record_data_with_post
    self.assertEqual(record.status, 500)
AttributeError: 'NoneType' object has no attribute 'status'

----------------------------------------------------------------------

It is unstable test, we should fix this now. Probably should you use find method here?

@@ -44,7 +44,7 @@ as root:

- Python version till 3.11 is supported, version **3.12 is NOT supported**
(we use [asyncore](https://docs.python.org/3.11/library/asyncore.html) that was removed in 3.12)

- ClickHouse Database 25 (Optional, used for storing logs in the database). See installation [here](https://clickhouse.com/docs/en/install#quick-install)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add info about creating the table into clickhouse here or TempestaFW wiki.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +162 to +165
if not messages:
return None

return messages[-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional:

Suggested change
if not messages:
return None
return messages[-1]
return messages[-1] or None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

>>> ll = []
>>> ll[-1] or None
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    ll[-1] or None
    ~~^^^^
IndexError: list index out of range

Comment on lines +151 to +154
if isinstance(self.log, bytes):
return AccessLogLine.parse_all(self.log.decode())

return AccessLogLine.parse_all(self.log)
Copy link
Contributor

Choose a reason for hiding this comment

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

optional:

Suggested change
if isinstance(self.log, bytes):
return AccessLogLine.parse_all(self.log.decode())
return AccessLogLine.parse_all(self.log)
return AccessLogLine.parse_all(self.log.decode() if isinstance(self.log, bytes) else self.log)

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 prefer to leave the code as it is because, to me, it looks more readable. However, I believe there’s a valid reason to create an issue regarding the invalid type of self.log, as it sometimes becomes a string or bytes. I wanted to address this immediately, but it would require verifying a lot of code, which makes it risky to do within this PR.

Comment on lines +187 to +188
self.loggers.dmesg.update()
self.assertEqual(self.loggers.dmesg.access_log_records_count(), 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

You should find method here. It is unstable test.

Comment on lines +127 to +134
self.send_simple_request(client)
self.assertEqual(self.loggers.dmesg.access_log_records_count(), 0)

tempesta = self.get_tempesta()
tempesta.stop()

pattern = r".*Starting daemon.*Daemon started.*Stopping daemon.*Daemon stopped.*"
self.assertTrue(self.loggers.clickhouse.find(pattern))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.send_simple_request(client)
self.assertEqual(self.loggers.dmesg.access_log_records_count(), 0)
tempesta = self.get_tempesta()
tempesta.stop()
pattern = r".*Starting daemon.*Daemon started.*Stopping daemon.*Daemon stopped.*"
self.assertTrue(self.loggers.clickhouse.find(pattern))
self.send_simple_request(client)
self.assertTrue(self.loggers.clickhouse.find("200")) # or other pattern for the requst
self.loggers.dmesg.update()
self.assertEqual(self.loggers.dmesg.access_log_records_count(), 0)

You must call update method for dmesg before access_log_* method or the log will be empty.

t_clickhouse/test_clickhouse_logs.py Outdated Show resolved Hide resolved
t_clickhouse/test_clickhouse_logs.py Outdated Show resolved Hide resolved
t_clickhouse/test_clickhouse_logs.py Show resolved Hide resolved
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.

3 participants