feat: Enable Logging functionality to both new and old Log Tables #657

Kevin101Zhang · 2024-04-10T20:12:13Z

Uncommented functionality so we actually start writing logs to new the Tables that have been provisioned in #643.
Old logging implementation remains untouched as still functions (although it has been renamed from writeLog -> writeLogOld). We are writing to both log tables.

1. Provisioning and Logging (to both tables) for a new Indexer

https://www.loom.com/share/3ad6d6ea3368412e8896340a74759ffb?sid=4d5379e8-5401-41bf-9e38-d0f8e8c4eca5

2. Logging (to both tables) for a existing Indexer

https://www.loom.com/share/4ba411f2bcb740e1842650f695ffb347?sid=253ced68-9d4c-459f-871b-b0a3ee00cd91

Provisioning and Logging new logs table for a existing Indexer (that does not have logs table)

https://www.loom.com/share/2aa7c0cc882f4dbdb9e51fc2a9e9b7b9?sid=1aa511fe-3054-4d27-9996-2b9fddc44ed8

removed useless test

…omnearqueryapipull608' of https://github.com/near/queryapi into 641-introduce-provisioning-of-logs

…omnearqueryapipull608' into 641-introduce-provisioning-of-logs

…om/near/queryapi into 641-introduce-provisioning-of-logs

runner/src/indexer/indexer.ts

morgsmccauley · 2024-04-11T21:20:01Z

runner/src/indexer/indexer.ts

+  async writeLog (logEntry: LogEntry, logEntries: LogEntry[], functionName: string): Promise<any> {
+    logEntries.push(logEntry);
+    const { level, blockHeight, message } = logEntry;
+    if (blockHeight) {


Why do we only call the old logging method if there is a blockHeight? Shouldn't we always call it?

Sorry, I should have made a comment here. I only added the if statement as a way of type asserting that bh exists. This is because the new log table allows for an optional 'bh' parameter, whereas the old method requires it. So when we call the writeLog and pass it an optional BH from LogEntry and attempt to call the old writeLogOld we just type assert it with an if statement.

One possible solution is to keep the current code as is, or alternatively, use a type assertion equivalent for the syntax. Another option is to backtrack the old 'writeLog' method and make the 'bh' parameter optional, but I don't think this is worth doing if we plan on removing the old methods. in the next PR

There's some confusion in your implementation about writeLog and writeLogOld. We ahve two separate functions. Are you trying to have writeLog also call writeLogOld? If so, writeLog should add to the log entries array and then call writeLogOld. And then we call writeLogs function on IndexerMeta at the end.

Or, if they're supposed to be separate, writeLog should not call writeLogOld. What's your intended implementation? The end result should be that ALL logs are added to the log entries array, and also calling writeLogOld. And then at the end, we run indexerMeta.writeLogs(THE LOGS ARRAY).

So, pick one:
A. writeLog adds to array, then calls writeLogOld
B. We only have writeLogOld and adding to logEntries.

Im confused about this comment here -.

The method writeLog's goal is to append a logEntry into logEntries[] and then call writeLogOld.

within direct block scope of runFunction we have access to both simultaneous promise array and logEntries array as they are declared locally.

Here we just push any log entries to an array for both to later be called in the finally so we do not use the writeLog method at all.

We start using writeLog in the other functional context - buildContext and buildDatabaseContext, originally they were just calling the old writeLog method-> now renamed to writeLogOld. This function has not changed besides the naming and its purpose is to run a graphQLQuery.

In the current code now we just added a param logEntries to both buildContext and buildDatabaseContext every time we want to write a log as the previous code want we just run it through the new writeLog method which does two things:

appends a log entry to the logEntries array to later be resolved in the finally block of runFunctions

call the writeLogOld to execute the graphQL query.

I believe its already doing option A well here unless im missing something

It looks odd as we explicitly call writeLogOld and then do logEntries.push earlier up (e.g. in Provisioning), and do writeLog() itself later on (e.g. context logs). I suppose this is fine?

I would like better would be to have writeLog take in a logEntry, a blockheight, logEntries array, and simultaneous promises array. You can add logEntry to the logEntries array (It may or may not have a BH). Then, you can add to simultaneous promises the call to writeLog Old which takes attributes of logEntry along with the passed in block height. This would result in writeLog fully encapsulating all our behavior, allowing us to keep writeLog calls exactly where they were originally while keeping the behavior. We also don't need to worry as much about optional blockheights. This is what I would consider to be a fully implemented option A.

Maybe @morgsmccauley can chime in here if I'm going off the track.

I understand where you are coming from. If this is permanent code I also believe that it would be great to pass in simultaneousPromises as a Param into buildcontext and builddatabasecontext methods and just push to the respective logEntries/simultaneous promises arrays. This way the functionality is strictly appending entries to be later resolved in the finally block in run functions. But then I thought since this is all temporary and the next PR on sprint would involve removing the hasura writes it did not make sense to continue.

Agree that this is a bit convoluted as we have two implementations going on. I would say let's defer tidying/optimising while this is in flux, and we can clean up once we remove the old implementation.

runner/src/indexer/indexer.test.ts

… to new _logs

runner/tests/integration.test.ts

runner/src/indexer/indexer.ts

Kevin101Zhang · 2024-04-15T21:51:10Z

runner/tests/integration.test.ts

+      }
+    `);
+
+    expect(provisioning_endpoints.length).toEqual(2);


{
"message": "Provisioning endpoint: starting"
},
{
"message": "Provisioning endpoint: successful"
}

Kevin101Zhang · 2024-04-15T21:57:04Z

runner/tests/integration.test.ts

+      }
+    `);
+
+    expect(running_function_enpoint.length).toEqual(2);


We run on 2 blocks
await indexer.execute(Block.fromStreamerMessage(block115185108 as any as StreamerMessage));
await indexer.execute(Block.fromStreamerMessage(block115185109 as any as StreamerMessage));

morgsmccauley

1 minor comment. Pre-emptively approving on the basis that mine/@darunrs comments are resolved before merging :).

morgsmccauley · 2024-04-16T03:17:23Z

runner/src/indexer/indexer.ts

+  async writeLog (logEntry: LogEntry, logEntries: LogEntry[], functionName: string): Promise<any> {
+    logEntries.push(logEntry);
+    const { level, blockHeight, message } = logEntry;
+    if (blockHeight) {


Agree that this is a bit convoluted as we have two implementations going on. I would say let's defer tidying/optimising while this is in flux, and we can clean up once we remove the old implementation.

morgsmccauley · 2024-04-16T03:18:12Z

runner/tests/integration.test.ts

@@ -149,6 +148,37 @@ describe('Indexer integration', () => {
    `);

    expect(logs.length).toEqual(4);
+
+    const { morgs_near_test___logs: _logs }: any = await graphqlClient.request(gql`


Suggested change

const { morgs_near_test___logs: _logs }: any = await graphqlClient.request(gql`

const { morgs_near_test___logs: logs }: any = await graphqlClient.request(gql`

The prefix here is really necessary, it's only really important at the DB level

I didn't use this naming due to the above code targeting indexer_log_entries
const { indexer_log_entries: logs }: any = await graphqlClient.request(gql

What I will do is rename indexer_log_entries: logs to indexer_log_entries: old_logs(Not the best name but easy to spot and remove next PR). This current variable can be called logs as suggested and will remove old_logs as apart of the PR to remove hasura writes

darunrs

Mostly good. Some comments to address. Can be merged after resolving the below comments.

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

runner/src/indexer-meta/indexer-meta.ts

runner/src/indexer/indexer.ts

darunrs · 2024-04-16T19:06:27Z

runner/src/indexer-meta/indexer-meta.ts

@@ -33,12 +32,11 @@ export default class IndexerMeta {
    const pgClient = pgClientInstance ?? new PgClient(databaseConnectionParameters);

    this.pgClient = pgClient;
-    this.schemaName = indexerConfig.schemaName();
-    this.loggingLevel = indexerConfig.logLevel;
+    this.indexerConfig = indexerConfig;


This is not necessary. Using private readonly will automatically expose indexerConfig to the class as a class variable.

Kevin101Zhang and others added 30 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

Update log-entry.test.ts

f4c7ccb

removed useless test

Update log-entry.test.ts

9de783f

uncommented out provisioning

89fa865

feat: added additional static classes in LogEntry

8d0da62

fix: modified indexer-logger and test to use logEntry

91a3d1b

fix: typo

0be44dd

Update log-entry.test.ts

8a331b2

chore: removed console.log

1809f7a

fix: LogLevel and LogType are defined in log-entry

7254a50

fix: indexer-logger now matches exact query

749e9f2

Merge branch '639-fix-unresolved-comments-in-original-pr-httpsgithubc…

00a6e61

…omnearqueryapipull608' of https://github.com/near/queryapi into 641-introduce-provisioning-of-logs

fix: added test without bh

a582921

Merge branch '639-fix-unresolved-comments-in-original-pr-httpsgithubc…

8459863

…omnearqueryapipull608' of https://github.com/near/queryapi into 641-introduce-provisioning-of-logs

merge scripts

0301061

fix: used fake timers on indexer-logger test

97e6ebc

fix: writeLog exclusively accepts an array of LogEntry

4e431e9

Merge branch '639-fix-unresolved-comments-in-original-pr-httpsgithubc…

6193ade

…omnearqueryapipull608' into 641-introduce-provisioning-of-logs

chore: moved log-entry files to indexer-logger

954528e

rebase

70c9153

rebase

16e9b69

feat: provisioning of new users

030fa4c

merge with main includes - feat: Provision logs for existing users

f7de470

chore: remove console.log

03ded3f

added back commented out test for metaDataTableDDL

8d48fc5

uncommented skip

80eb9f1

Merge branch '641-introduce-provisioning-of-logs' of https://github.c…

451b5bc

…om/near/queryapi into 641-introduce-provisioning-of-logs

uncommented skip, removed redundant test cases

e097cbc

Kevin101Zhang marked this pull request as ready for review April 10, 2024 22:51

Kevin101Zhang requested a review from a team as a code owner April 10, 2024 22:51

morgsmccauley requested changes Apr 11, 2024

View reviewed changes

feat: added unit test to check logs, removed unused line

d68a242

Kevin101Zhang commented Apr 14, 2024

View reviewed changes

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

feat: added integration test for logs asserting log_entries are equal…

d6285fe

… to new _logs

Kevin101Zhang commented Apr 14, 2024

View reviewed changes

runner/tests/integration.test.ts Outdated Show resolved Hide resolved

Kevin101Zhang requested a review from morgsmccauley April 15, 2024 16:43

darunrs reviewed Apr 15, 2024

View reviewed changes

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

darunrs reviewed Apr 15, 2024

View reviewed changes

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

Kevin101Zhang added 4 commits April 15, 2024 16:40

rebase

2ba9ffd

fix: corrected param for indexerMeta instantiation

d5f87de

fix: use IndexerConfig for indexerMeta

6ed96aa

searching for provisioning endpoints

11dfe35

Kevin101Zhang commented Apr 15, 2024

View reviewed changes

fix: check run functions

b960b2c

Kevin101Zhang commented Apr 15, 2024

View reviewed changes

morgsmccauley approved these changes Apr 16, 2024

View reviewed changes

Kevin101Zhang added 2 commits April 16, 2024 11:40

chore: renamed log table

a3a040e

chore: spacing

845cfac

darunrs approved these changes Apr 16, 2024

View reviewed changes

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

runner/src/indexer-meta/indexer-meta.ts Outdated Show resolved Hide resolved

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

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

Kevin101Zhang added 2 commits April 16, 2024 13:54

set systemLogs in buildDB context to user

1ead54b

fix: use of IndexerConfig in indexerMeta

4e718d6

Kevin101Zhang merged commit e23d4c1 into main Apr 16, 2024
3 checks passed

Kevin101Zhang deleted the 650-release-logs-to-production-both-old-and-new-way branch April 16, 2024 18:55

darunrs reviewed Apr 16, 2024

View reviewed changes

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

Kevin101Zhang changed the title ~~Enable Logging functionality to both new and old Log Tables~~ feat: Enable Logging functionality to both new and old Log Tables Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable Logging functionality to both new and old Log Tables #657

feat: Enable Logging functionality to both new and old Log Tables #657

Kevin101Zhang commented Apr 10, 2024 •

edited

Loading

morgsmccauley Apr 11, 2024

Kevin101Zhang Apr 12, 2024 •

edited

Loading

darunrs Apr 15, 2024

Kevin101Zhang Apr 15, 2024

Kevin101Zhang Apr 15, 2024

darunrs Apr 15, 2024

Kevin101Zhang Apr 16, 2024

morgsmccauley Apr 16, 2024

Kevin101Zhang Apr 15, 2024

Kevin101Zhang Apr 15, 2024

morgsmccauley left a comment

morgsmccauley Apr 16, 2024

morgsmccauley Apr 16, 2024

Kevin101Zhang Apr 16, 2024 •

edited

Loading

darunrs left a comment •

edited

Loading

darunrs Apr 16, 2024

	const { morgs_near_test___logs: _logs }: any = await graphqlClient.request(gql`
	const { morgs_near_test___logs: logs }: any = await graphqlClient.request(gql`

feat: Enable Logging functionality to both new and old Log Tables #657

feat: Enable Logging functionality to both new and old Log Tables #657

Conversation

Kevin101Zhang commented Apr 10, 2024 • edited Loading

1. Provisioning and Logging (to both tables) for a new Indexer

2. Logging (to both tables) for a existing Indexer

Provisioning and Logging new logs table for a existing Indexer (that does not have logs table)

Choose a reason for hiding this comment

Kevin101Zhang Apr 12, 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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kevin101Zhang Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

darunrs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kevin101Zhang commented Apr 10, 2024 •

edited

Loading

Kevin101Zhang Apr 12, 2024 •

edited

Loading

Kevin101Zhang Apr 16, 2024 •

edited

Loading

darunrs left a comment •

edited

Loading