Skip to content
Draft
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions server.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,13 @@
import './src/utils/instrumentation';
import {fileURLToPath} from 'url';
import {initializeApp} from './src/initializeApp';

const isMainModule = process.argv[1] === fileURLToPath(import.meta.url);
const port: number = parseInt(process.env.PORT || '3000', 10);
const isWorkerMode = process.env.WORKER_MODE === 'true';

// Load the appropriate app once
let app;
if (isWorkerMode) {
const workerModule = await import('./src/worker-app');
app = workerModule.default;
} else {
const appModule = await import('./src/app');
app = appModule.default;
}
// Create an instance of the appropriate app
const app = await initializeApp({isWorkerMode});

// Start the server if this file is run directly
if (isMainModule) {
Expand All @@ -35,5 +29,4 @@ if (isMainModule) {
start();
}

// Export the app
export default app;
export default app;
42 changes: 0 additions & 42 deletions src/app.ts

This file was deleted.

5 changes: 5 additions & 0 deletions src/initializeApp.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export async function initializeApp({isWorkerMode}: {isWorkerMode: boolean}) {
const appModulePath = isWorkerMode ? './src/worker-app' : './src/service-app';
const appModule = await import(appModulePath);
return appModule.default();
}
46 changes: 46 additions & 0 deletions src/service-app.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Main module file
import fastify from 'fastify';
import {TypeBoxTypeProvider} from '@fastify/type-provider-typebox';
import loggingPlugin from './plugins/logging';
import corsPlugin from './plugins/cors';
import proxyPlugin from './plugins/proxy';
import {getLoggerConfig} from './utils/logger';
import {errorHandler} from './utils/error-handler';
import v1Routes from './routes/v1';
import replyFrom from '@fastify/reply-from';

function createApp() {
const app = fastify({
logger: getLoggerConfig(),
disableRequestLogging: true,
trustProxy: process.env.TRUST_PROXY !== 'false'
}).withTypeProvider<TypeBoxTypeProvider>();

// Register global validation error handler
app.setErrorHandler(errorHandler());

// Register reply-from plugin
app.register(replyFrom);

// Register CORS plugin
app.register(corsPlugin);

// Register logging plugin
app.register(loggingPlugin);

// Register proxy plugin
app.register(proxyPlugin);

// Register v1 routes
app.register(v1Routes, {prefix: '/api/v1'});

// Routes
app.get('/', async () => {
return 'Hello World - Github Actions Deployment Test';
});

return app;
}

export default createApp;

15 changes: 11 additions & 4 deletions src/services/events/publisher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ class EventPublisher {
private static instance: EventPublisher;
private pubsub: PubSub;

private constructor() {
this.pubsub = new PubSub({
private constructor(pubsub?: PubSub) {
this.pubsub = pubsub || new PubSub({
projectId: process.env.GOOGLE_CLOUD_PROJECT
});
}
Expand All @@ -24,6 +24,10 @@ class EventPublisher {
return EventPublisher.instance;
}

static resetInstance(pubsub?: PubSub): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

reset is confusing me, but just a tiny bit :) maybe we could say here, createInstance ? it makes the intent more explicit or something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah no disagreement. I also don't love this pattern in general — it's sort of trying to be dependency injection, but it's not really. I'll have more of a think about this.

Copy link
Contributor

@ibalosh ibalosh Jul 4, 2025

Choose a reason for hiding this comment

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

Gotcha, ok, so I see you used Singleton pattern to create a single instance and reuse it over time. If I can read the intention in the code correctly, you use this single instance across in proxy. If that is the case, and it's always the same that makes sense.

But the reset seems out of place, and only used in tests. It could be taken out of this class in some ways, and used only in tests, or maybe something along the lines:

const googleProjectId = process.env.GOOGLE_CLOUD_PROJECT;
const productionPubSub = new PubSub({ projectId: googleProjectId });

class EventPublisher {
    private pubsub: PubSub;

    constructor(pubsub: PubSub) {}
    // Remove all singleton logic
}

const defaultPublisher = new EventPublisher(productionPubSub);

// Main API - used by production code
export const publishEvent = async (options: PublishEventOptions): Promise<string> => {
    return defaultPublisher.publishEvent(options);
};

// For tests - create with mock
export { EventPublisher }; 

In this case it's closer to the Dependency injection idea. Just thinking out loud as I was checking it out, feel free to check the comment out, you can resolve it too. Don't want to take too much time in the review from you.

EventPublisher.instance = new EventPublisher(pubsub);
}

async publishEvent({topic, payload, logger}: PublishEventOptions): Promise<string> {
try {
const message = {
Expand All @@ -32,7 +36,7 @@ class EventPublisher {
};

const messageId = await this.pubsub.topic(topic).publishMessage(message);

logger.info({
messageId,
topic,
Expand All @@ -55,4 +59,7 @@ class EventPublisher {
export const publishEvent = async ({topic, payload, logger}: PublishEventOptions): Promise<string> => {
const publisher = EventPublisher.getInstance();
return publisher.publishEvent({topic, payload, logger});
};
};

// Export for testing purposes
export {EventPublisher};
44 changes: 24 additions & 20 deletions src/worker-app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,29 @@ import loggingPlugin from './plugins/logging';
import workerPlugin from './plugins/worker-plugin';
import {getLoggerConfig} from './utils/logger';

const app = fastify({
logger: getLoggerConfig(),
disableRequestLogging: true,
trustProxy: process.env.TRUST_PROXY !== 'false'
});
function createApp() {
const app = fastify({
logger: getLoggerConfig(),
disableRequestLogging: true,
trustProxy: process.env.TRUST_PROXY !== 'false'
});

// Register logging plugin for consistent log formatting
app.register(loggingPlugin);

// Register worker plugin for heartbeat logging
app.register(workerPlugin);

// Health endpoints for Cloud Run deployment
app.get('/', async () => {
return {status: 'worker-healthy'};
});

app.get('/health', async () => {
return {status: 'worker-healthy'};
});

// Register logging plugin for consistent log formatting
app.register(loggingPlugin);
return app;
}

// Register worker plugin for heartbeat logging
app.register(workerPlugin);

// Health endpoints for Cloud Run deployment
app.get('/', async () => {
return {status: 'worker-healthy'};
});

app.get('/health', async () => {
return {status: 'worker-healthy'};
});

export default app;
export default createApp;
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
import {describe, it, expect, beforeAll} from 'vitest';
import {FastifyInstance} from 'fastify';
import {initializeApp} from '../../../../src/initializeApp';

describe('/api/v1/page_hit', () => {
let fastify: FastifyInstance;
let app: FastifyInstance;

beforeAll(async function () {
const appModule = await import('../../../../src/app');
fastify = appModule.default;
await fastify.ready();
app = await initializeApp({isWorkerMode: false});
});

it('should return 200', async function () {
const response = await fastify.inject({
const response = await app.inject({
Copy link
Contributor

Choose a reason for hiding this comment

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

focus outside of implementation 👌 nice touch 👍

method: 'GET',
url: '/api/v1/page_hit'
});
Expand Down
88 changes: 88 additions & 0 deletions test/integration/api/web_analytics/batch-mode.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import {describe, expect, it, beforeEach, beforeAll, vi} from 'vitest';
import {FastifyInstance, FastifyRequest, FastifyReply} from 'fastify';
import {expectResponse} from '../../../utils/assertions';
import {createPubSubSpy} from '../../../utils/pubsub-spy';
import defaultValidRequestQuery from '../../../utils/fixtures/defaultValidRequestQuery.json';
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe nicer if we organize these with index.ts?

The more our code describes intentions (in this case, kind of the package, which would be fixtures) , the better.

Whatcha think?

import {
  defaultValidRequestQuery,
  defaultValidRequestHeaders,
  defaultValidRequestBody,
  headersWithoutSiteUuid
} from '../../../utils/fixtures';

import defaultValidRequestHeaders from '../../../utils/fixtures/defaultValidRequestHeaders.json';
import defaultValidRequestBody from '../../../utils/fixtures/defaultValidRequestBody.json';
import headersWithoutSiteUuid from '../../../utils/fixtures/headersWithoutSiteUuid.json';
import {initializeApp} from '../../../../src/initializeApp';
import {EventPublisher} from '../../../../src/services/events/publisher';

const preHandlerStub = async (_request: FastifyRequest, reply: FastifyReply) => {
reply.code(202);
};

describe('POST /tb/web_analytics', () => {
let app: FastifyInstance;

describe('Batch Mode - Publishing to pub/sub', function () {
let pubSubSpy: ReturnType<typeof createPubSubSpy>;
const pageHitsRawTopic: string = 'page-hits-raw';

beforeAll(async function () {
vi.stubEnv('PUBSUB_TOPIC_PAGE_HITS_RAW', pageHitsRawTopic);
});
beforeEach(async function () {
app = await initializeApp({isWorkerMode: false});
app.addHook('preHandler', preHandlerStub);
pubSubSpy = createPubSubSpy();
EventPublisher.resetInstance(pubSubSpy.mockPubSub as any);
});

it('should transform the request body and publish to pub/sub', async function () {
await app.inject({
method: 'POST',
url: '/tb/web_analytics',
query: defaultValidRequestQuery,
headers: defaultValidRequestHeaders,
body: defaultValidRequestBody
});

pubSubSpy.expectPublishedMessageToTopic(pageHitsRawTopic).withMessageData({
Copy link
Contributor

Choose a reason for hiding this comment

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

up until here, I think it's fine, readable. you arrange and act. But here, couple of problems:

expectation is moved outside of the test

That can be fine, when intentions are clear, like the assertion you wrote for response. It's clearly visible you will assert responses, and in test itself, viewer already know mostly what you are asserting against.

In this case however, you moved the whole assertion logic, with conditionals to the spy, which is acting more as asserting framework, rather than just being a spy. Also, you are asserting waaay to many things there. I did that too before, so I know this feeling :) you just want to make it reusable, I understand.

But... What I would suggest is to go simple, move that logic back to the test. Also, go small, maybe you could try to test with same data, but add multiple tests and assert separate pieces: data, number of calls, timestamp. It's ok if they are separate tests, and there are repeated things, as long as test is readable. We could think about moving it back, fully, and start from there, see how it looks and start decoupling.

At the moment, I am not 100% sure what the test is doing, I do see the assertion name, but I am not sure what it does, and conditionals there confuse the reader. Tests should read simple.

I think that if you need multiple method names, and conditions in helper, it's a sign that test could be decoupled in multiple smaller intentional tests. You don't want to build in a separate DSL for assertion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling we don't need separate spy util at all

timestamp: expect.any(String),
action: 'page_hit',
version: '1',
site_uuid: defaultValidRequestHeaders['x-site-uuid'],
payload: {
event_id: defaultValidRequestBody.payload.event_id,
href: 'https://www.example.com/',
pathname: '/',
member_uuid: 'undefined',
member_status: 'undefined',
post_uuid: 'undefined',
post_type: 'null',
parsedReferrer: {
medium: '',
source: '',
url: ''
},
locale: 'en-US',
location: 'US',
referrer: null
},
meta: {
ip: expect.any(String),
'user-agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/138.0.0.0 Safari/537.36'
}
});
});

it('should not publish a message if the request fails validation', async function () {
const response = await app.inject({
method: 'POST',
url: '/tb/web_analytics',
query: defaultValidRequestQuery,
headers: headersWithoutSiteUuid,
body: defaultValidRequestBody
});
expectResponse({
response,
statusCode: 400,
errorType: 'Bad Request',
message: 'headers must have required property \'x-site-uuid\''
});
pubSubSpy.expectNoMessagesPublished();
});
});
});
Loading
Loading