feat: log geo location info to InfluxDB #339
Conversation
|
Seems the build-api step is failing because it's missing the GLIF_TOKEN env variable. Not sure how my changes would have caused this |
|
Link to roadmap issue: space-meridian/roadmap#112 |
PatrickNercessian
force-pushed
the
feat-log-network-info
branch
from
6bb447c
to
87acfa3
Compare
api/lib/network-info-logger.js
Outdated
| * @param {import('node:http').IncomingMessage} req | ||
| * @param {string} stationId | ||
| * @param {string} inetGroup | ||
| * @param {function} recordTelemetryFn |
There was a problem hiding this comment.
It would be great to use a typed function here. See how we do it in spark-evaluate:
There was a problem hiding this comment.
Would you want a new file like in spark-evaluate for the typings? Or just have it in telemetry.js or network-info-logger.js?
There was a problem hiding this comment.
I prefer to add common/typings.d.ts file for consistency with the rest of our projects.
There was a problem hiding this comment.
In other places, we use the variable/argument name recordTelemetry. Please do the same here for consistency.
publish/test/test.js
Outdated
|
|
||
| import { assertApproximately } from '../../test-helpers/assert.js' | ||
|
|
||
| const { DATABASE_URL } = process.env | ||
|
|
||
| after(telemetry.close) | ||
| after(async () => { await telemetry.publishWriteClient.close() }) |
There was a problem hiding this comment.
IIRC, we are calling close to prevent the write client from blocking the process from exiting.
Ideally, tests should not write any telemetry, they should mock the recordTelemetryFn, as we already do in other tests.
Alternatively, the telemetry module should implement a close function to close all writers.
For example (may not work out of the box):
// usage here - no change
after(telemetry.close)
// implementation in common/telemetry
export const close = async () => {
await Promise.all([
publishWriteClient.close(),
apiWriterClient.close(),
//etc.
])
}There was a problem hiding this comment.
This closing logic was already present I think, this change is just to bring it in line with the new telemetry file.
If we want to change this, maybe it's better scoped in a different PR?
There was a problem hiding this comment.
I see your point.
The problem I am trying to prevent is that with your changes in place, the test suite does not exit cleanly, because there are background active writers.
Before your change, when executing the tests in publish/test/test.js, only one writer was created and this writer was closed by this after line.
With your changes in place, executing this test file creates more than on writer, but we close only one of them. As a result, the test suite never exits.
I confirmed this by running the following command:
npm t -w api -- -i -g "getRoundStartEpoch|startRoundTracker"
We want to rework this test file anyway; it should not write any telemetry, as I mentioned before. Essentially, we want to port filecoin-station/voyager-api#22 to this repo.
I am going to make two smaller changes: fix this test file to use a mocked telemetry recorder and introduce common/typings.d.ts file as discussed in #339 (comment)
There was a problem hiding this comment.
See #354
In the end, I did not need common/typings.d.ts, so I am leaving that change for you to make in this pull request.
There was a problem hiding this comment.
| after(async () => { await telemetry.publishWriteClient.close() }) |
PatrickNercessian
force-pushed
the
feat-log-network-info
branch
from
c2b1e2d
to
3e21450
Compare
| @@ -1,7 +1,7 @@ | |||
| // This file is shared with voyager-api/voyager-publish | |||
| // Helpers in this file must not have anything project-specific like measurement fields | |||
|
|
|||
| import { Point } from '../publish/lib/telemetry.js' | |||
| import { Point } from '../common/telemetry.js' | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Addressed in 6332977
| @@ -51,6 +52,9 @@ try { | |||
| process.exit(1) | |||
| } | |||
|
|
|||
| // Clear the station IDs seen by the network info logger every 24 hours | |||
| setInterval(clearNetworkInfoStationIdsSeen, 1000 * 60 * 60 * 24) | |||
|
I created #360 to run the test on CI under my account, all passed 👍🏻 |
bajtos
changed the title
No description provided.