fix: Unresolved comments in #608 #640

Kevin101Zhang · 2024-04-05T18:36:07Z

Feat: created logEntry class and test cases
Chore: relocated createLogs to abstracted func
Chore: renamed schema idx to prefix with '__'

removed useless test

darunrs · 2024-04-05T21:54:17Z

runner/src/log-entry/log-entry.ts

@@ -0,0 +1,22 @@
+import { LogType, LogLevel } from '../indexer-logger/indexer-logger';


I see you created the class but where do you use it? I'm expecting to see it used somewhere. Even if you have to update Indexer and then comment it out again. It at least lets us know you ran it locally and it was still working.

Im under the impression after my last sync with @morgsmccauley that because this is going to role out in a specific order this is strictly for resolving the issues # 1.

Fix all unresolved comments in original PR
Prefix log indexes with __
Move creation of logs table to abstracted function
Update LogEntry to be a class

Uncomment provisioning of logs, add unit tests in Provisioner, and ensure integration tests pass

Ensure auto-provisioning of logs for existing users works, and merge feat: Provision logs for existing users #636

Uncomment logging calls, update unit tests in Indexer, and ensure integration tests pass

Because its rolling out in a specific order I would have to modify the indexer no matter what. Is there a reason we have to do it in step 1 as oppose to step 4. I would be modifying the indexer file regardless of it being commented out that would pollute this current PR by editing all the files again at once despite being commented out

You don't have to actually implement test and release that code. It was more like, we are adding this class, was it actually used locally as a test? To make sure it does everything we need? You can just re-comment out indexer again afterward.

You mention below you used it so that's all good.

I would go ahead and start using this class, even if the final result is commented out. We can worry about tests etc later, and just use TypeScript to guide/provide confidence. This will reduce the size of future PRs making things a bit easier.

I integrated the LogEntry class and validated its functionality within the indexer-logger test suites. I have additional comments for the indexer.ts file itself but they are not in this pr. Do let me know if I should include them. I was going to introduce them in beginning stages of step 4 to keep the modifications small despite being commented out.

runner/src/log-entry/log-entry.test.ts

runner/src/log-entry/log-entry.ts

runner/src/provisioner/provisioner.ts

morgsmccauley · 2024-04-07T21:22:40Z

runner/src/log-entry/log-entry.ts

@@ -0,0 +1,22 @@
+import { LogType, LogLevel } from '../indexer-logger/indexer-logger';


This code is tightly coupled with IndexerLogger and is small enough that I think it could just exist within IndexerLogger - we will never use one without the other. Can we please move it there?

Yeah I agree it is tightly if not directly dependent and unusable without IndexerLogger. But I think until we have the full picture of LogEntry keeping it separate allows for easier management and potential future expansions. Towards the end of the Epic if there is still not purpose of keeping them separate I think we can move it back with few issues.

We can still keep a separate file, but house it under indexer-logger/. It's only purpose is to be used within indexer-logger so it makes sense to have there.

I want to reduce the noise in the root directory. As this code base starts to grow we should be creating higher level abstractions, housing related functionality, to make navigation easier.

runner/src/log-entry/log-entry.test.ts

runner/src/log-entry/log-entry.ts

morgsmccauley · 2024-04-07T21:40:35Z

runner/src/log-entry/log-entry.ts

@@ -0,0 +1,22 @@
+import { LogType, LogLevel } from '../indexer-logger/indexer-logger';


I would go ahead and start using this class, even if the final result is commented out. We can worry about tests etc later, and just use TypeScript to guide/provide confidence. This will reduce the size of future PRs making things a bit easier.

morgsmccauley · 2024-04-07T21:41:48Z

Can you please update the PR title to something like fix: Unresolved comments in #608

morgsmccauley · 2024-04-07T21:42:19Z

@darunrs were there any other comments from you on #608 that should be resolved here?

runner/src/log-entry/log-entry.test.ts

Kevin101Zhang · 2024-04-08T17:31:57Z

runner/src/log-entry/log-entry.ts

+    this.timestamp = new Date();
+  }
+
+  static createLog (message: string, level: LogLevel, type: LogType, blockHeight?: number): LogEntry {


Introduced a static create method that takes all the necessary parameters for creating a LogEntry instance. This method eliminates the redundancy. Each static method (systemDebug, systemInfo, systemWarn, etc) now just calls the create method with appropriate params, reducing duplication and making the code more DRY.

Kevin101Zhang · 2024-04-08T17:39:35Z

runner/src/indexer-logger/indexer-logger.test.ts

-      };
-
-      await indexerLogger.writeLogs(logEntry);
+      const debugEntry = LogEntry.systemDebug('Debug message');


The IndexerLogger class is created with a specified logging level, and a log entry with a lower level is generated using the newly added LogEntry class.

darunrs

Pretty close! I don't have any comments from 608 which needs to be addressed anymore here. Just some comments on data organization and such. Can approve after remaining comments are resolved.

runner/src/indexer-logger/indexer-logger.ts

runner/src/indexer-logger/indexer-logger.test.ts

runner/src/indexer-logger/indexer-logger.ts

runner/src/log-entry/log-entry.test.ts

runner/src/indexer/indexer.test.ts

Kevin101Zhang · 2024-04-08T21:55:08Z

runner/src/indexer-logger/indexer-logger.test.ts

@@ -30,8 +30,23 @@ describe('IndexerLogger', () => {
      const infoEntry = LogEntry.systemInfo('Info message');
      await indexerLogger.writeLogs(infoEntry);

-      const expectedQueryStructure = `INSERT INTO "${functionName}".__logs (block_height, date, timestamp, type, level, message) VALUES`;
-      expect(query.mock.calls[0][0]).toContain(expectedQueryStructure);
+      const timestampPattern = '\\d{4}-\\d{2}-\\d{2} \\d{2}:\\d{2}:\\d{2}\\.\\d+\\+\\d{2}';


I went with the regex approach. Im sure there is also one where we specify the timestamp

const specificDate = new Date(1673491200000) // March 15, 2023 00:00:00 UTC
specificDate.toISOString()

I would not advise you use complicated things like regex's. Especially when testing correctness. Please use a hardcoded value. Unit tests are where you would want to text hard coded values as you can easily tell what soemthing is supposed to look like and what part of it doesn't match, if it doesnt. If someone told me my unit test failed because it didn't match some arbitrary regex, I'd have no clue why, what is different, and what needs to be fixed. And I would need to then understand this regex too.

Mocking the timestamp for LogEntry instances may be difficult without modifying the constructor to accept an optional timestamp parameter. Since the timestamp is initialized within the constructor with the current system time by default.

constructor ( public readonly message: string, public readonly level: LogLevel, public readonly type: LogType, public readonly blockHeight?: number ) { this.timestamp = new Date(); }

unless we make timestamp not a readonly or something like this

constructor( public readonly message: string, public readonly level: LogLevel, public readonly type: LogType, public readonly blockHeight?: number, timestamp?: Date // Optional parameter for specifying the timestamp ) { if (timestamp) { this.timestamp = timestamp; // Use the provided timestamp if available } else { this.timestamp = new Date(); // Otherwise, use the current system time }

I mentioned in a previous comment we can use Fake Timers, looks like you resolved it but didn't actually implement? Can you make sure to only resolve if you have followed up?

We don't need to inject the Date/timestamp, we can mock the date like so:

it('should insert a single log entry into the database', async () => { const date = new Date(); jest.useFakeTimers({ now: date.getTime() }); const indexerLogger = new IndexerLogger(functionName, LogLevel.INFO, mockDatabaseConnectionParameters, pgClient); await indexerLogger.writeLogs(LogEntry.systemInfo('Info message')); const expectedDate = date.toISOString().replace('T', ' ').replace('Z', '+00'); expect(query.mock.calls[0][0]) .toEqual(`INSERT INTO "${functionName}".__logs (block_height, date, timestamp, type, level, message) VALUES (NULL, '${expectedDate}', '${expectedDate}', 'system', 'INFO', 'Info message')`); });

You may need to mock the Date at the top level, and pass the mocked date value around to avoid mock conflicts across tests.

Also note I had to change the Date format because pgFormat changes it under the hood. The alternative would be to use format here in the test to get the same result.

Adding an optional field to the parameter for the purpose of testing isn't outlandish. We do it quite often in fact. Honestly, you can also just say .contains( THE PART AFTER THE TIMESTAMP). The point I was making was we don't validate the query contains all of the expected values. As long as you expect on it, it's fine.

Validating the timestamp in a unit test is a separate problem, and I'm ok not mixing them.

@morgsmccauley Ah I must have tunneled there and resolved it after I reading the initial comment. I didn't realize that implementation need jest to mock. Ill go ahead and mock it with FakeTimers

morgsmccauley · 2024-04-08T23:40:34Z

runner/src/indexer-logger/indexer-logger.ts

@@ -57,7 +40,7 @@ export default class IndexerLogger {
  async writeLogs (
    logEntries: LogEntry | LogEntry[],
  ): Promise<void> {
-    const entriesArray = (Array.isArray(logEntries) ? logEntries : [logEntries]).filter(entry => this.shouldLog(entry.logLevel)); ;
+    const entriesArray = (Array.isArray(logEntries) ? logEntries : [logEntries]).filter(entry => this.shouldLog(entry.level)); ;


On the previous PR I mentioned changing this writeLogs method to only take LogEntry[] so we don't need this logic. Can we go ahead and make that change please?

darunrs

LGTM! It does require a large rebase, so go ahead and do that and I
l'll approve after skimming it over!

Kevin101Zhang · 2024-04-09T20:06:04Z

Incorporating Latest Changes in main to branch

First few commits is from this branch #640. Created this branch based off the initial branch. This PR intends to introduce the creation of the logs table by provisioning the logsSchema and the follow CRON jobs for new Users but does not use or writeLogs to the new logsTable itself. If this is merged by itself new users will have unused log table but the provisioning will occur. To provision existing users - #636

Kevin101Zhang added 3 commits April 4, 2024 18:09

fix: relocated runLogsSql into setupPartitionLogsTable

4ada070

chore: add prefix for INDEXs in schema

8b57a79

feat: added log-entry class and test

a709dae

Kevin101Zhang requested a review from a team as a code owner April 5, 2024 18:36

Kevin101Zhang linked an issue Apr 5, 2024 that may be closed by this pull request

Fix unresolved comments in original PR (https://github.com/near/queryapi/pull/608) #639

Closed

Kevin101Zhang requested a review from darunrs April 5, 2024 18:37

Kevin101Zhang added 2 commits April 5, 2024 14:49

Update log-entry.test.ts

f4c7ccb

removed useless test

Update log-entry.test.ts

9de783f

Kevin101Zhang mentioned this pull request Apr 5, 2024

feat: Introduce provisioning of Logs Table for new and existing users #643

Merged

darunrs reviewed Apr 5, 2024

View reviewed changes

morgsmccauley reviewed Apr 7, 2024

View reviewed changes

Kevin101Zhang changed the title ~~639 fix unresolved comments in original pr httpsgithubcomnearqueryapipull608~~ fix: Unresolved comments in #608 Apr 8, 2024

Kevin101Zhang added 2 commits April 8, 2024 13:05

feat: added additional static classes in LogEntry

8d0da62

fix: modified indexer-logger and test to use logEntry

91a3d1b

Kevin101Zhang commented Apr 8, 2024

View reviewed changes

runner/src/log-entry/log-entry.test.ts Outdated Show resolved Hide resolved

Kevin101Zhang commented Apr 8, 2024

View reviewed changes

Kevin101Zhang requested a review from darunrs April 8, 2024 18:06

Kevin101Zhang added 3 commits April 8, 2024 14:47

fix: typo

0be44dd

Update log-entry.test.ts

8a331b2

chore: removed console.log

1809f7a

darunrs reviewed Apr 8, 2024

View reviewed changes

Kevin101Zhang added 2 commits April 8, 2024 17:18

fix: LogLevel and LogType are defined in log-entry

7254a50

fix: indexer-logger now matches exact query

749e9f2

darunrs reviewed Apr 8, 2024

View reviewed changes

runner/src/indexer/indexer.test.ts Outdated Show resolved Hide resolved

Kevin101Zhang commented Apr 8, 2024

View reviewed changes

fix: added test without bh

a582921

morgsmccauley reviewed Apr 8, 2024

View reviewed changes

Kevin101Zhang added 2 commits April 9, 2024 03:40

fix: used fake timers on indexer-logger test

97e6ebc

fix: writeLog exclusively accepts an array of LogEntry

4e431e9

Kevin101Zhang requested review from darunrs and morgsmccauley April 9, 2024 07:48

darunrs reviewed Apr 9, 2024

View reviewed changes

Kevin101Zhang added 2 commits April 9, 2024 15:12

chore: moved log-entry files to indexer-logger

954528e

rebase

70c9153

morgsmccauley approved these changes Apr 9, 2024

View reviewed changes

darunrs approved these changes Apr 9, 2024

View reviewed changes

Kevin101Zhang merged commit 89dadf5 into main Apr 9, 2024
3 checks passed

Kevin101Zhang deleted the 639-fix-unresolved-comments-in-original-pr-httpsgithubcomnearqueryapipull608 branch April 9, 2024 22:45

This was referenced Apr 22, 2024

Prod Release 23/04/24 #686

Merged

test stable branch git fix up #687

Closed

git fixup test 2 #688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Unresolved comments in #608 #640

fix: Unresolved comments in #608 #640

Kevin101Zhang commented Apr 5, 2024

darunrs Apr 5, 2024

Kevin101Zhang Apr 5, 2024 •

edited

Loading

darunrs Apr 6, 2024

morgsmccauley Apr 7, 2024

Kevin101Zhang Apr 8, 2024 •

edited

Loading

morgsmccauley Apr 7, 2024

Kevin101Zhang Apr 8, 2024

morgsmccauley Apr 8, 2024

morgsmccauley Apr 7, 2024

morgsmccauley commented Apr 7, 2024

morgsmccauley commented Apr 7, 2024

Kevin101Zhang Apr 8, 2024

Kevin101Zhang Apr 8, 2024 •

edited

Loading

darunrs left a comment

Kevin101Zhang Apr 8, 2024

darunrs Apr 8, 2024

Kevin101Zhang Apr 8, 2024

morgsmccauley Apr 8, 2024 •

edited

Loading

darunrs Apr 8, 2024

Kevin101Zhang Apr 9, 2024

morgsmccauley Apr 8, 2024

darunrs left a comment

Kevin101Zhang commented Apr 9, 2024 •

edited

Loading

		@@ -0,0 +1,22 @@
		import { LogType, LogLevel } from '../indexer-logger/indexer-logger';

fix: Unresolved comments in #608 #640

fix: Unresolved comments in #608 #640

Conversation

Kevin101Zhang commented Apr 5, 2024

Choose a reason for hiding this comment

Kevin101Zhang Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kevin101Zhang Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morgsmccauley commented Apr 7, 2024

morgsmccauley commented Apr 7, 2024

Choose a reason for hiding this comment

Kevin101Zhang Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

darunrs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morgsmccauley Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darunrs left a comment

Choose a reason for hiding this comment

Kevin101Zhang commented Apr 9, 2024 • edited Loading

Kevin101Zhang Apr 5, 2024 •

edited

Loading

Kevin101Zhang Apr 8, 2024 •

edited

Loading

Kevin101Zhang Apr 8, 2024 •

edited

Loading

morgsmccauley Apr 8, 2024 •

edited

Loading

Kevin101Zhang commented Apr 9, 2024 •

edited

Loading