Fixes to make fetch Join work in CLI including use name over nameToFilePath and replacing / to . in MetaData names
#398
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThis pull request refines several modules across online, spark, and API components. It enhances error handling in the KVStore, improves configuration handling by introducing stricter type checks and a new helper case class, and standardizes key name generation by shifting from file path to direct metadata name usage. Additionally, constants and helper methods have been added to support uniformity, and tests have been updated accordingly. Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
david-zlai
commented
Feb 19, 2025
| new MetadataEndPoint[Conf]( | ||
| extractFn = (path, conf) => (path.confPathToKey, ThriftJsonCodec.toJsonStr(conf)), | ||
| // extractFn = (path, conf) => (path.confPathToKey, ThriftJsonCodec.toJsonStr(conf)), | ||
| extractFn = (metadataName, conf) => (metadataName, ThriftJsonCodec.toJsonStr(conf)), |
Contributor
Author
There was a problem hiding this comment.
changing this to metadataName because metadataName is sourced from the actual conf:
{
"metaData": {
"name": "quickstart.training_set.v1",
....
Previously, path.confPathToKey extracts from the conf path. It's better to just be consistent and use the name in the actual conf (not the confpath)
david-zlai
commented
Feb 19, 2025
| val startNs = System.nanoTime | ||
| val requests = Seq(Fetcher.Request(featureName, keyMap, args.atMillis.toOption)) | ||
| val resultFuture = if (args.confType() == "join") { | ||
| val resultFuture = if (args.confType() == "joins") { |
Contributor
Author
There was a problem hiding this comment.
this change is all over the place.
so confType gets sourced from run.py. and it is set from the directories that chronon expects like joins, group_bys...
and is used in this ROUTES map: https://github.com/zipline-ai/chronon/blob/main/api/py/ai/chronon/repo/run.py#L94-L121
I'm making this plural here to be consistent
Contributor
There was a problem hiding this comment.
should we put make this ConfType and object like Constants with Joins, GroupBys etc?
david-zlai
force-pushed
the
davidhan/debug_fetch
branch
4 times, most recently
from
4c4d6a1 to
614cb4b
Compare
david-zlai
commented
Feb 19, 2025
Comment on lines
86
to
88
| case 0 => { | ||
| Failure(new RuntimeException(s"No values returned from KVStore. " + | ||
| s"Request for key=${key} in dataset=${dataset} failed")) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
saw this happen when calling Fetch from cli:
david-zlai
changed the title
david-zlai
force-pushed
the
davidhan/debug_fetch
branch
from
8e167c6 to
96e985b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
online/src/main/scala/ai/chronon/online/FetcherMain.scala (1)
43-43:⚠️ Potential issueFix inconsistent default value.
The default value "join" doesn't match the plural form used in choices ("joins", "group_bys").
- choice(Seq("joins", "group_bys"), required = false, descr = "the type of conf to fetch", default = Some("join")) + choice(Seq("joins", "group_bys"), required = false, descr = "the type of conf to fetch", default = Some("joins"))
🧹 Nitpick comments (3)
online/src/main/scala/ai/chronon/online/MetadataDirWalker.scala (1)
127-128: Remove commented out code.The old implementation is tracked in version control and doesn't need to be preserved in comments.
-// .extractFn(filePath, conf.asInstanceOf[api.Model]) - .extractFn(metadataName.get, conf.asInstanceOf[api.Model]) + .extractFn(metadataName.get, conf.asInstanceOf[api.Model])cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigTableKVStoreImpl.scala (2)
129-130: Enhance log message structure.Add more context to the log message for better traceability.
- logger.info("Querying for dataset: " + request.dataset) + logger.info(s"BigTable query: dataset=${request.dataset}")
152-152: Good use of StandardCharsets.UTF_8.The explicit charset is good practice. Consider structuring the log message better.
- logger.info(s"Querying key: " + new String(baseRowKey, StandardCharsets.UTF_8)) + logger.info(s"BigTable query: rowKey=${new String(baseRowKey, StandardCharsets.UTF_8)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigTableKVStoreImpl.scala(3 hunks)online/src/main/scala/ai/chronon/online/Api.scala(1 hunks)online/src/main/scala/ai/chronon/online/FetcherMain.scala(3 hunks)online/src/main/scala/ai/chronon/online/MetadataDirWalker.scala(2 hunks)online/src/main/scala/ai/chronon/online/MetadataEndPoint.scala(1 hunks)online/src/main/scala/ai/chronon/online/MetadataStore.scala(2 hunks)spark/src/main/scala/ai/chronon/spark/Driver.scala(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- spark/src/main/scala/ai/chronon/spark/Driver.scala
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: join_tests
- GitHub Check: non_spark_tests
- GitHub Check: streaming_tests
- GitHub Check: groupby_tests
- GitHub Check: fetcher_tests
- GitHub Check: analyzer_tests
- GitHub Check: spark_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (14)
online/src/main/scala/ai/chronon/online/MetadataEndPoint.scala (1)
54-54: LGTM! Good parameter renaming.Parameter name better reflects the data source.
online/src/main/scala/ai/chronon/online/MetadataStore.scala (1)
181-183: LGTM!Enhanced error handling to catch and log all potential errors.
online/src/main/scala/ai/chronon/online/MetadataDirWalker.scala (5)
81-101: LGTM! Improved metadata name handling.The tuple destructuring and metadata name extraction from configuration enhances consistency.
103-103: LGTM! Better null safety.The additional check for metadataName.isDefined improves robustness.
81-101: Well-structured tuple return value.The change to return both config and metadata name improves error handling consistency.
103-103: Good defensive programming.Checking both optConf and metadataName prevents potential null pointer exceptions.
112-128: Consistent use of metadata name.The change from filePath to metadataName.get aligns with the new error handling structure.
online/src/main/scala/ai/chronon/online/FetcherMain.scala (3)
42-43: LGTM! Better type safety with choice.Restricting confType to valid values prevents runtime errors.
189-189: LGTM! Better parameter naming.Using named parameter improves code clarity.
189-203: Improved code clarity.Named parameter and consistent plural form enhance readability.
online/src/main/scala/ai/chronon/online/Api.scala (2)
85-91: LGTM! Better error handling for empty results.The explicit check for empty values with clear error message improves debugging.
85-91: Better error handling for empty results.Explicit handling of empty values with informative error messages improves debugging.
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigTableKVStoreImpl.scala (2)
40-40: LGTM! Added required import.StandardCharsets import supports the new logging functionality.
129-130: LGTM! Enhanced logging.Added logging improves visibility of dataset queries and key lookups.
Also applies to: 152-152
david-zlai
commented
Feb 19, 2025
| lazy val getJoinConf: TTLCache[String, Try[JoinOps]] = new TTLCache[String, Try[JoinOps]]( | ||
| { name => | ||
| val startTimeMs = System.currentTimeMillis() | ||
| val result = getConf[Join](s"joins/$name") |
Contributor
Author
There was a problem hiding this comment.
So I made this change only because it's not compatible with how I've done the run.py + dataprocsubmitter at the moment.
to explain, when we do metadata upload for a join what happens is that we:
- first take the join conf file like
production/joins/quickstart/training_set.v1and then upload it to gcsgs://zipline-warehouse-canary/metadata/production/joins/quickstart/training_set.v1 - next, we add that as a fileUri to dataproc so that dataproc copies the gcs file over to the spark working directory
dataproc will copy the file but that only copies production/joins/quickstart/ training_set.v1 . dataproc won't (or from what I see won't) preserve any part of the original path. so training_set.v1 file is at the working directory.
(Chronon would have traditionally expected production/joins/quickstart/training_set.v1 and then called confPathToKey returning **joins**/quickstart/training_set.v1)
- and later in the code, we'll upload the join conf to BigTable's
CHRONON_METADATAwhere we'll set the key based on the path of the conf. And since the path of the conf from dataproc is justtraining_set.v1, it'll look like this in bt:
........this breaks when we try to ultimately do a fetch join now as on this line of code we hardcode s"joins/$name"
nikhil-zlai
reviewed
Feb 19, 2025
Contributor
nikhil-zlai
left a comment
nikhil-zlai
left a comment
There was a problem hiding this comment.
i think we should match everywhere to the python import path - even the compiled files should.
production/joins.team.file.var
in cli fetch we no longer need type just get joins.team.file.var
and metadata upload should write to the key joins.team.file.var
wdyt?
| .filter(Filters.FILTERS.family().exactMatch(ColumnFamilyString)) | ||
| .filter(Filters.FILTERS.qualifier().exactMatch(ColumnFamilyQualifierString)) | ||
|
|
||
| logger.info("Querying for dataset: " + request.dataset) |
Contributor
There was a problem hiding this comment.
lets not log in the hot path - this will kill the disk on the box
Contributor
Author
There was a problem hiding this comment.
oh crap, forgot this is hot path. removing
| case _ => | ||
| // for non-timeseries data, we just look up based on the base row key | ||
| val baseRowKey = buildRowKey(request.keyBytes, request.dataset) | ||
| logger.info(s"Querying key: " + new String(baseRowKey, StandardCharsets.UTF_8)) |
| val startNs = System.nanoTime | ||
| val requests = Seq(Fetcher.Request(featureName, keyMap, args.atMillis.toOption)) | ||
| val resultFuture = if (args.confType() == "join") { | ||
| val resultFuture = if (args.confType() == "joins") { |
Contributor
There was a problem hiding this comment.
should we put make this ConfType and object like Constants with Joins, GroupBys etc?
piyush-zlai
reviewed
Feb 19, 2025
Contributor
piyush-zlai
left a comment
piyush-zlai
left a comment
There was a problem hiding this comment.
Couple of minor comments
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigTableKVStoreImpl.scala
Outdated
Show resolved
Hide resolved
nikhil-zlai
approved these changes
Feb 19, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
online/src/main/scala/ai/chronon/online/ConfKeywordConstants.scala (1)
4-7: Add scaladoc for each constant.Document the purpose and valid usage of each configuration key.
case object ConfKeywordConstants { + /** Configuration key for join definitions */ val JoinKeyword = "joins" + /** Configuration key for group by definitions */ val GroupByKeyword = "group_bys" + /** Configuration key for staging query definitions */ val StagingQueryKeyword = "staging_queries" + /** Configuration key for model definitions */ val ModelKeyword = "models" }online/src/main/scala/ai/chronon/online/Extensions.scala (1)
57-79: Extract common key name generation logic.Identical pattern repeated across classes.
+ private def generateKeyName(keyword: String, name: String): String = + s"$keyword/" + name implicit class JoinOps(join: Join) { - def keyNameForKvStore: String = { - s"$JoinKeyword/" + join.metaData.name - } + def keyNameForKvStore: String = generateKeyName(JoinKeyword, join.metaData.name) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
online/src/main/scala/ai/chronon/online/Api.scala(1 hunks)online/src/main/scala/ai/chronon/online/ConfKeywordConstants.scala(1 hunks)online/src/main/scala/ai/chronon/online/Extensions.scala(2 hunks)online/src/main/scala/ai/chronon/online/FetcherMain.scala(3 hunks)online/src/main/scala/ai/chronon/online/MetadataDirWalker.scala(3 hunks)online/src/main/scala/ai/chronon/online/MetadataStore.scala(6 hunks)spark/src/main/scala/ai/chronon/spark/Driver.scala(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- online/src/main/scala/ai/chronon/online/MetadataDirWalker.scala
- online/src/main/scala/ai/chronon/online/FetcherMain.scala
- spark/src/main/scala/ai/chronon/spark/Driver.scala
- online/src/main/scala/ai/chronon/online/Api.scala
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: streaming_tests
- GitHub Check: analyzer_tests
- GitHub Check: spark_tests
- GitHub Check: join_tests
- GitHub Check: groupby_tests
- GitHub Check: non_spark_tests
- GitHub Check: fetcher_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (7)
online/src/main/scala/ai/chronon/online/ConfKeywordConstants.scala (1)
3-8: LGTM! Good use of case object for type-safe constants.online/src/main/scala/ai/chronon/online/MetadataStore.scala (5)
27-27: LGTM! Well-structured configuration handling.The new case class provides type-safe configuration handling with proper validation.
Also applies to: 45-59
82-105: LGTM! Improved type safety.Pattern matching for configuration types enhances type safety and maintainability.
152-152: LGTM! Fixed join configuration key handling.The change correctly handles join configuration keys in the dataproc environment.
208-210: LGTM! Enhanced error handling.Added broader error logging while maintaining existing behavior.
173-176: LGTM! Consistent key generation.Key generation now aligns with the new configuration handling approach.
online/src/main/scala/ai/chronon/online/Extensions.scala (1)
20-22: LGTM: Imports are well-organized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/main/scala/ai/chronon/api/Extensions.scala (1)
446-448: Consider extracting common key name generation logic.All
keyNameForKvStoreimplementations follow the same pattern. Consider extracting to a common trait or utility function.+trait KeyNameGenerator { + def metaData: MetaData + def keywordType: String + def keyNameForKvStore: String = s"$keywordType/" + metaData.name +}Also applies to: 834-836, 1230-1234, 1236-1240
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
api/src/main/scala/ai/chronon/api/ConfKeywordConstants.scala(1 hunks)api/src/main/scala/ai/chronon/api/Extensions.scala(4 hunks)online/src/main/scala/ai/chronon/online/Extensions.scala(2 hunks)online/src/main/scala/ai/chronon/online/FetcherMain.scala(4 hunks)online/src/main/scala/ai/chronon/online/MetadataDirWalker.scala(3 hunks)online/src/main/scala/ai/chronon/online/MetadataStore.scala(6 hunks)spark/src/main/scala/ai/chronon/spark/Driver.scala(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- online/src/main/scala/ai/chronon/online/Extensions.scala
- online/src/main/scala/ai/chronon/online/MetadataStore.scala
- online/src/main/scala/ai/chronon/online/MetadataDirWalker.scala
- online/src/main/scala/ai/chronon/online/FetcherMain.scala
- spark/src/main/scala/ai/chronon/spark/Driver.scala
🔇 Additional comments (2)
api/src/main/scala/ai/chronon/api/ConfKeywordConstants.scala (1)
1-8: LGTM!Well-structured constants using case object pattern.
api/src/main/scala/ai/chronon/api/Extensions.scala (1)
19-22: LGTM!Clean import organization for the new constants.
david-zlai
force-pushed
the
davidhan/debug_fetch
branch
from
a7398e3 to
7699ae2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/main/scala/ai/chronon/api/Extensions.scala (1)
448-450: Consider consolidating duplicate code using a trait.The
keyNameForKvStoremethod is implemented identically across multiple classes. This could be refactored into a trait to reduce code duplication.+trait KeyNameForKvStore { + def metaData: MetaData + def keywordType: String + def keyNameForKvStore: String = Extensions._keyNameForKvStore(metaData, keywordType) +} -implicit class GroupByOps(groupBy: GroupBy) extends GroupBy(groupBy) { +implicit class GroupByOps(groupBy: GroupBy) extends GroupBy(groupBy) with KeyNameForKvStore { + override def keywordType: String = GroupByKeyword - def keyNameForKvStore: String = { - _keyNameForKvStore(groupBy.metaData, GroupByKeyword) - } } -implicit class JoinOps(val join: Join) extends Serializable { +implicit class JoinOps(val join: Join) extends Serializable with KeyNameForKvStore { + override def keywordType: String = JoinKeyword - def keyNameForKvStore: String = { - _keyNameForKvStore(join.metaData, JoinKeyword) - } } -implicit class StagingQueryOps(stagingQuery: StagingQuery) { +implicit class StagingQueryOps(stagingQuery: StagingQuery) with KeyNameForKvStore { + override def keywordType: String = StagingQueryKeyword - def keyNameForKvStore: String = { - _keyNameForKvStore(stagingQuery.metaData, StagingQueryKeyword) - } } -implicit class ModelOps(model: Model) { +implicit class ModelOps(model: Model) with KeyNameForKvStore { + override def keywordType: String = ModelKeyword - def keyNameForKvStore: String = { - _keyNameForKvStore(model.metaData, ModelKeyword) - } }Also applies to: 836-838, 1232-1236, 1238-1242
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (14)
api/src/main/scala/ai/chronon/api/Extensions.scala(5 hunks)online/src/main/scala/ai/chronon/online/Fetcher.scala(1 hunks)online/src/main/scala/ai/chronon/online/MetadataStore.scala(6 hunks)online/src/main/scala/ai/chronon/online/stats/DriftStore.scala(1 hunks)spark/src/main/scala/ai/chronon/spark/LogFlattenerJob.scala(3 hunks)spark/src/main/scala/ai/chronon/spark/scripts/ObservabilityDemo.scala(2 hunks)spark/src/main/scala/ai/chronon/spark/stats/ConsistencyJob.scala(1 hunks)spark/src/main/scala/ai/chronon/spark/stats/drift/Summarizer.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/SchemaEvolutionTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/analyzer/DerivationTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/bootstrap/LogBootstrapTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/fetcher/ChainingFetcherTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/fetcher/FetcherTest.scala(3 hunks)spark/src/test/scala/ai/chronon/spark/test/stats/drift/DriftTest.scala(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- spark/src/test/scala/ai/chronon/spark/test/SchemaEvolutionTest.scala
- spark/src/test/scala/ai/chronon/spark/test/bootstrap/LogBootstrapTest.scala
- spark/src/test/scala/ai/chronon/spark/test/stats/drift/DriftTest.scala
- spark/src/main/scala/ai/chronon/spark/LogFlattenerJob.scala
- online/src/main/scala/ai/chronon/online/stats/DriftStore.scala
- spark/src/main/scala/ai/chronon/spark/stats/ConsistencyJob.scala
- spark/src/test/scala/ai/chronon/spark/test/fetcher/ChainingFetcherTest.scala
- spark/src/main/scala/ai/chronon/spark/scripts/ObservabilityDemo.scala
- spark/src/test/scala/ai/chronon/spark/test/analyzer/DerivationTest.scala
- online/src/main/scala/ai/chronon/online/Fetcher.scala
- spark/src/main/scala/ai/chronon/spark/stats/drift/Summarizer.scala
- online/src/main/scala/ai/chronon/online/MetadataStore.scala
- spark/src/test/scala/ai/chronon/spark/test/fetcher/FetcherTest.scala
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: streaming_tests
- GitHub Check: analyzer_tests
- GitHub Check: spark_tests
- GitHub Check: join_tests
- GitHub Check: fetcher_tests
- GitHub Check: groupby_tests
- GitHub Check: non_spark_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (2)
api/src/main/scala/ai/chronon/api/Extensions.scala (2)
19-22: LGTM: Imports are well-organized.The imports are correctly grouped and follow a logical order.
46-48: LGTM: Well-structured helper method.The private helper method provides a consistent way to generate key names across different components.
david-zlai
force-pushed
the
davidhan/debug_fetch
branch
from
7699ae2 to
bbcab9e
Compare
david-zlai
force-pushed
the
davidhan/debug_fetch
branch
from
bbcab9e to
73fc856
Compare
david-zlai
force-pushed
the
davidhan/debug_fetch
branch
from
db29ae4 to
ff3a913
Compare
david-zlai
changed the title
name over nameToFilePath and replacing / to . in MetaData names
david-zlai
commented
Feb 21, 2025
| val metrics: Metrics.Context = Metrics.Context(Metrics.Environment.JoinLogFlatten, joinConf) | ||
|
|
||
| private def getUnfilledRanges(inputTable: String, outputTable: String): Seq[PartitionRange] = { | ||
| val partitionName: String = joinConf.metaData.nameToFilePath.replace("/", "%2F") |
Contributor
Author
There was a problem hiding this comment.
this one line killed me. not sure why we did this but I think it's because later on when we do SHOW PARTITIONS from hive and collect the partition values, when we store values with / in them they get encoded into %2F. so I guess we wanted to be consistent and did this hack.
I'm deleting this line and instead making sure we just use metaData.name which should come without /'s in them
piyush-zlai
approved these changes
Feb 21, 2025
4 tasks
ken-zlai
added a commit
that referenced
this pull request
Feb 25, 2025
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected? ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallback label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
kumar-zlai
pushed a commit
that referenced
this pull request
Feb 26, 2025
…FilePath and replacing `/` to `.` in MetaData names (#398) ## Summary ^^^ Tested on the etsy laptop. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to explicitly report when configuration values are missing. - **New Features** - Introduced standardized constants for various configuration types, ensuring consistent key naming. - **Refactor** - Unified metadata processing by using direct metadata names instead of file paths. - Enhanced type safety in configuration options for clearer and more reliable behavior. - **Tests** - Updated test cases and parameters to reflect the improved metadata and configuration handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
kumar-zlai
pushed a commit
that referenced
this pull request
Feb 26, 2025
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected? ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallback label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 tasks
kumar-zlai
pushed a commit
that referenced
this pull request
Apr 25, 2025
…FilePath and replacing `/` to `.` in MetaData names (#398) ## Summary ^^^ Tested on the etsy laptop. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to explicitly report when configuration values are missing. - **New Features** - Introduced standardized constants for various configuration types, ensuring consistent key naming. - **Refactor** - Unified metadata processing by using direct metadata names instead of file paths. - Enhanced type safety in configuration options for clearer and more reliable behavior. - **Tests** - Updated test cases and parameters to reflect the improved metadata and configuration handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
kumar-zlai
pushed a commit
that referenced
this pull request
Apr 25, 2025
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected? ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallback label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
kumar-zlai
pushed a commit
that referenced
this pull request
Apr 29, 2025
…FilePath and replacing `/` to `.` in MetaData names (#398) ## Summary ^^^ Tested on the etsy laptop. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to explicitly report when configuration values are missing. - **New Features** - Introduced standardized constants for various configuration types, ensuring consistent key naming. - **Refactor** - Unified metadata processing by using direct metadata names instead of file paths. - Enhanced type safety in configuration options for clearer and more reliable behavior. - **Tests** - Updated test cases and parameters to reflect the improved metadata and configuration handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
kumar-zlai
pushed a commit
that referenced
this pull request
Apr 29, 2025
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected? ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallback label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 tasks
chewy-zlai
pushed a commit
that referenced
this pull request
May 15, 2025
…FilePath and replacing `/` to `.` in MetaData names (#398) ## Summary ^^^ Tested on the our clients laptop. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to explicitly report when configuration values are missing. - **New Features** - Introduced standardized constants for various configuration types, ensuring consistent key naming. - **Refactor** - Unified metadata processing by using direct metadata names instead of file paths. - Enhanced type safety in configuration options for clearer and more reliable behavior. - **Tests** - Updated test cases and parameters to reflect the improved metadata and configuration handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
chewy-zlai
pushed a commit
that referenced
this pull request
May 15, 2025
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected? ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallback label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
chewy-zlai
pushed a commit
that referenced
this pull request
May 15, 2025
…FilePath and replacing `/` to `.` in MetaData names (#398) ## Summary ^^^ Tested on the our clients laptop. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to explicitly report when configuration values are missing. - **New Features** - Introduced standardized constants for various configuration types, ensuring consistent key naming. - **Refactor** - Unified metadata processing by using direct metadata names instead of file paths. - Enhanced type safety in configuration options for clearer and more reliable behavior. - **Tests** - Updated test cases and parameters to reflect the improved metadata and configuration handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
chewy-zlai
pushed a commit
that referenced
this pull request
May 15, 2025
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected? ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallback label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
chewy-zlai
pushed a commit
that referenced
this pull request
May 16, 2025
…FilePath and replacing `/` to `.` in MetaData names (#398) ## Summary ^^^ Tested on the our clients laptop. ## Cheour clientslist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to explicitly report when configuration values are missing. - **New Features** - Introduced standardized constants for various configuration types, ensuring consistent key naming. - **Refactor** - Unified metadata processing by using direct metadata names instead of file paths. - Enhanced type safety in configuration options for clearer and more reliable behavior. - **Tests** - Updated test cases and parameters to reflect the improved metadata and configuration handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
chewy-zlai
pushed a commit
that referenced
this pull request
May 16, 2025
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Doour clientser images and rebuild on this branch to confirm observability works as expected? ## Cheour clientslist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallbaour clients label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
^^^
Tested on the etsy laptop.
Checklist
Summary by CodeRabbit