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.

Sign up for GitHub

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

Jump to bottom

[DO NOT MERGE] feat: switch to memcache-client over memcached #6119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+57 −84
Open

[DO NOT MERGE] feat: switch to memcache-client over memcached #6119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
"lodash": "4.17.21",
"longjohn": "0.2.12",
"marked": "2.0.1",
"memcached": "2.2.2",
"memcache-client": "^1.0.5",
"moment": "2.29.4",
"moment-timezone": "0.5.37",
"node-fetch": "2.6.7",
Expand Down
45 changes: 0 additions & 45 deletions src/lib/__tests__/cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ jest.mock("lib/tracer", () => {
cacheTracer: {
get: jest.fn((promise) => promise),
set: jest.fn((promise) => promise),
delete: jest.fn((promise) => promise),
},
}
})
Expand Down Expand Up @@ -38,28 +37,6 @@ describe("Cache with compression enabled", () => {
})
})

describe("#delete", () => {
beforeEach(async () => await cache.set("get_foo", { bar: "baz" }))

it("deletes the data", async () => {
expect.assertions(1)

await cache.delete("get_foo")

let data

try {
data = await cache.get("get_foo")

throw new Error("unexpected error")
} catch {
// no-op
}

expect(data).toBeUndefined()
})
})

describe("#set", () => {
describe("with a plain Object", () => {
it("sets the cache and includes a timestamp", async () => {
Expand Down Expand Up @@ -117,28 +94,6 @@ describe("Cache with compression disabled", () => {
})
})

describe("#delete", () => {
beforeEach(async () => await cache.set("get_foo", { bar: "baz" }))

it("deletes the data", async () => {
expect.assertions(1)

await cache.delete("get_foo")

let data

try {
data = await cache.get("get_foo")

throw new Error("unexpected error")
} catch {
// no-op
}

expect(data).toBeUndefined()
})
})

describe("#set", () => {
describe("with a plain Object", () => {
it("sets the cache and includes a timestamp", async () => {
Expand Down
54 changes: 18 additions & 36 deletions src/lib/cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import zlib from "zlib"
import { isArray } from "lodash"
import config from "config"
import { error, verbose } from "./loggers"
import Memcached from "memcached"
import { MemcacheClient } from "memcache-client"
import { createCacheTracer } from "./tracer"
import { createStatsClient } from "./stats"

Expand All @@ -18,14 +18,6 @@ const {

const isTest = NODE_ENV === "test"

const VerboseEvents = [
"issue",
"failure",
"reconnecting",
"reconnect",
"remove",
]

const uncompressedKeyPrefix = "::"
const cacheVersion = "v1"

Expand Down Expand Up @@ -68,19 +60,17 @@ function createMockClient() {
}

function createMemcachedClient() {
const client = new Memcached(MEMCACHED_URL, {
poolSize: MEMCACHED_MAX_POOL,
})
VerboseEvents.forEach((event) => {
client.on(event, () => verbose(`[Cache] ${event}`))
console.log("hey")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

was just making sure this was a singleton

Copy link
Contributor

Choose a reason for hiding this comment

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

👋

const newClient = new MemcacheClient({
server: { server: MEMCACHED_URL, maxConnections: MEMCACHED_MAX_POOL },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if maxConnections is this pool config, there doesn't seem to be any connection pool config in the new lib

Copy link
Contributor

Choose a reason for hiding this comment

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

It does seem to be an internal maximum on the number of connections made by this client. (I was worried it might be a server option due to its location.)

})
return client
return newClient
}

export const client = isTest ? createMockClient() : createMemcachedClient()

const cacheTracer: ReturnType<typeof createCacheTracer> = isTest
? { get: (x) => x, set: (x) => x, delete: (x) => x }
? { get: (x) => x, set: (x) => x }
: createCacheTracer()

const statsClient = isTest ? null : createStatsClient()
Expand Down Expand Up @@ -108,15 +98,18 @@ function _get<T>(key) {
reject(err)
} else if (data) {
if (CACHE_COMPRESSION_DISABLED) {
resolve(JSON.parse(data.toString()))
resolve(JSON.parse(data.value.toString()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

one API change, the returned data is an object with some metadata, value contains the actual cached object.

} else {
zlib.inflate(Buffer.from(data, "base64"), (er, inflatedData) => {
if (er) {
reject(er)
} else {
resolve(JSON.parse(inflatedData.toString()))
zlib.inflate(
Buffer.from(data.value, "base64"),
(er, inflatedData) => {
if (er) {
reject(er)
} else {
resolve(JSON.parse(inflatedData.toString()))
}
}
})
)
}
} else {
reject()
Expand Down Expand Up @@ -146,7 +139,7 @@ function _set(key, data, options: CacheOptions) {
return new Promise<void>((resolve, reject) => {
const payload = JSON.stringify(data)
verbose(`CACHE SET: ${cacheKey(key)}: ${payload}`)
client.set(cacheKey(key), payload, cacheTtl, (err) => {
client.set(cacheKey(key), payload, { lifetime: cacheTtl }, (err) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

slight difference for set

err ? reject(err) : resolve()
})
}).catch(error)
Expand All @@ -157,7 +150,7 @@ function _set(key, data, options: CacheOptions) {
verbose(`CACHE SET: ${cacheKey(key)}: ${payload}`)

return new Promise<void>((resolve, reject) => {
client.set(cacheKey(key), payload, cacheTtl, (err) =>
client.set(cacheKey(key), payload, { lifetime: cacheTtl }, (err) =>
err ? reject(err) : resolve()
)
})
Expand All @@ -166,13 +159,6 @@ function _set(key, data, options: CacheOptions) {
}
}

const _delete = (key) =>
new Promise<void>((resolve, reject) =>
client.del(cacheKey(key), (err) => {
err ? reject(err) : resolve()
})
)

export default {
get: <T = any>(key: string) => {
return cacheTracer.get(_get<T>(key))
Expand All @@ -181,8 +167,4 @@ export default {
set: (key: string, data: any, options: CacheOptions = {}) => {
return cacheTracer.set(_set(key, data, options))
},

delete: (key: string) => {
return cacheTracer.delete(_delete(key))
},
}
1 change: 0 additions & 1 deletion src/lib/tracer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,5 @@ export function createCacheTracer() {
return {
get: createCommand("get"),
set: createCommand("set"),
delete: createCommand("delete"),
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 didn't see any use case in the codebase outside of tests for delete, and you see we renamed client.del to delete. Which seems non-standard as both clients implement del, and that's what the rate limit middleware wants as well - https://github.com/express-rate-limit/rate-limit-memcached?tab=readme-ov-file#client

So perhaps the del part of rate limiter, whatever that was, never quite worked? Then again that middleware is disabled anyway

}
}
39 changes: 38 additions & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7323,6 +7323,11 @@ lodash.deburr@^4.1.0:
resolved "https://registry.yarnpkg.com/lodash.deburr/-/lodash.deburr-4.1.0.tgz#ddb1bbb3ef07458c0177ba07de14422cb033ff9b"
integrity sha1-3bG7s+8HRYwBd7oH3hRCLLAz/5s=

[email protected]:
version "4.2.0"
resolved "https://registry.yarnpkg.com/lodash.defaults/-/lodash.defaults-4.2.0.tgz#d09178716ffea4dde9e5fb7b37f6f0802274580c"
integrity sha512-qjxPLHd3r5DnsdGacqOMU6pb/avJzdh9tFX2ymgoZE27BmjXrNy/y4LoaiTeAb+O3gL8AfpJGtqfX/ae2leYYQ==

lodash.find@^4.6.0:
version "4.6.0"
resolved "https://registry.yarnpkg.com/lodash.find/-/lodash.find-4.6.0.tgz#cb0704d47ab71789ffa0de8b97dd926fb88b13b1"
Expand Down Expand Up @@ -7556,7 +7561,22 @@ [email protected]:
resolved "https://registry.yarnpkg.com/media-typer/-/media-typer-0.3.0.tgz#8710d7af0aa626f8fffa1ce00168545263255748"
integrity sha1-hxDXrwqmJvj/+hzgAWhUUmMlV0g=

[email protected], memcached@^2.2.2:
memcache-client@^1.0.5:
version "1.0.5"
resolved "https://registry.yarnpkg.com/memcache-client/-/memcache-client-1.0.5.tgz#7a2eb1f1924b8a87ef7f80a25f42949da4d1eff7"
integrity sha512-hUADvh+Cju3eNZ5AWGfNhdI/Y4251pc8Hxq+6RryiVNzMeEnXKQ2z+gyBBAPY4SZnC1nOtp2MJUSsnCWB9t6lg==
dependencies:
lodash.defaults "4.2.0"
memcache-parser "^1.0.0"
optional-require "1.1.8"
tslib "2.1.0"

memcache-parser@^1.0.0:
version "1.0.1"
resolved "https://registry.yarnpkg.com/memcache-parser/-/memcache-parser-1.0.1.tgz#66b8434e1bccd79a6da4bfe5fec41e5aa6f585b8"
integrity sha512-F5TjjHKUNMCWTVfiJD+7M9ecNbBdnK6xifYZ1WL8PoQ/dP9sB37VQV8hPtyuUqJJFj76qY70Ezj9saQ0gw2FJg==

memcached@^2.2.2:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the rate limit middleware still brings in memcached...maybe we should just clean that middleware up?

Copy link
Contributor

Choose a reason for hiding this comment

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

I dug through slack for some history and couldn't figure out the state of this, or if it was ever enabled. Ideally, rate-limit-memcached should be removed (and re-added if/when needed). I also wonder if the presence of both libraries could interfere with Datadog's tracing.

Which reminds me: we must confirm that datadog automatically instruments this alternate library.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do remove rate-limit-memcached, we can also remove the mock increment implementation in cache.ts above.

version "2.2.2"
resolved "https://registry.yarnpkg.com/memcached/-/memcached-2.2.2.tgz#68f86ccfd84bcf93cc25ed46d6d7fc0c7521c9d5"
integrity sha1-aPhsz9hLz5PMJe1G1tf8DHUhydU=
Expand Down Expand Up @@ -8279,6 +8299,13 @@ opentracing@>=0.12.1:
resolved "https://registry.yarnpkg.com/opentracing/-/opentracing-0.14.4.tgz#a113408ea740da3a90fde5b3b0011a375c2e4268"
integrity sha512-nNnZDkUNExBwEpb7LZaeMeQgvrlO8l4bgY/LvGNZCR0xG/dGWqHqjKrAmR5GUoYo0FIz38kxasvA1aevxWs2CA==

[email protected]:
version "1.1.8"
resolved "https://registry.yarnpkg.com/optional-require/-/optional-require-1.1.8.tgz#16364d76261b75d964c482b2406cb824d8ec44b7"
integrity sha512-jq83qaUb0wNg9Krv1c5OQ+58EK+vHde6aBPzLvPPqJm89UQWsvSuFy9X/OSNJnFeSOKo7btE0n8Nl2+nE+z5nA==
dependencies:
require-at "^1.0.6"

optionator@^0.8.1:
version "0.8.2"
resolved "https://registry.yarnpkg.com/optionator/-/optionator-0.8.2.tgz#364c5e409d3f4d6301d6c0b4c05bba50180aeb64"
Expand Down Expand Up @@ -9236,6 +9263,11 @@ [email protected], request@^2.83.0:
tunnel-agent "^0.6.0"
uuid "^3.3.2"

require-at@^1.0.6:
version "1.0.6"
resolved "https://registry.yarnpkg.com/require-at/-/require-at-1.0.6.tgz#9eb7e3c5e00727f5a4744070a7f560d4de4f6e6a"
integrity sha512-7i1auJbMUrXEAZCOQ0VNJgmcT2VOKPRl2YGJwgpHpC9CE91Mv4/4UYIUm4chGJaI381ZDq1JUicFii64Hapd8g==

require-directory@^2.1.1:
version "2.1.1"
resolved "https://registry.yarnpkg.com/require-directory/-/require-directory-2.1.1.tgz#8c64ad5fd30dab1c976e2344ffe7f792a6a6df42"
Expand Down Expand Up @@ -10409,6 +10441,11 @@ tsconfig-paths@^3.9.0:
minimist "^1.2.0"
strip-bom "^3.0.0"

[email protected]:
version "2.1.0"
resolved "https://registry.yarnpkg.com/tslib/-/tslib-2.1.0.tgz#da60860f1c2ecaa5703ab7d39bc05b6bf988b97a"
integrity sha512-hcVC3wYEziELGGmEEXue7D75zbwIIVUMWAVbHItGPx0ziyXxrOMQx4rQEVEV45Ut/1IotuEvwqPopzIOkDMf0A==

tslib@^1.10.0:
version "1.13.0"
resolved "https://registry.yarnpkg.com/tslib/-/tslib-1.13.0.tgz#c881e13cc7015894ed914862d276436fa9a47043"
Expand Down