Conversation
sarthaksin1857
force-pushed
the
fix-graphite-datagen
branch
from
f89d7e2 to
8f70419
Compare
Fix data generation script to send required fields nit remove non graphite changes nit Add datagen changes Fails still but can deploy local code Set defaults for all variables and manually specify pub_time Add table on supported metadata field nit Only confirm pub_time is set properly
documentation fixup
sarthaksin1857
force-pushed
the
fix-graphite-datagen
branch
from
ec4a46e to
adc9f02
Compare
|
@Gerrrr Can you take a look here |
Gerrrr
left a comment
Gerrrr
left a comment
There was a problem hiding this comment.
Thanks for opening this PR, @sarthaksin1857 ! I found a few things we need to fix before it is ready to merge. Also, given the wealth of subtle issues, this class may deserve a few unit tests.
| filter data by commit or version using `--since-commit` or `--since-version` selectors. | ||
|
|
||
| #### Supported Metadata Schema | ||
| The following keys are supported within the `data` dictionary: |
There was a problem hiding this comment.
It may not be clear what is the data dictionary in this case. In the paragraph above, we talk about tags, so I suggest replacing this statement with The following tags are supported:
| branch: Optional[str] | ||
| commit: Optional[str] | ||
|
|
||
| def __init__( |
There was a problem hiding this comment.
there is something funky going on with indentation. Intelij highlights edits in this commit differently from the rest of the file:
| if isinstance(self.pub_time, str): | ||
| self.pub_time = parse_datetime(self.pub_time) | ||
| elif isinstance(self.pub_time, (int, float)): | ||
| self.pub_time = datetime.fromtimestamp(self.pub_time) |
There was a problem hiding this comment.
This change uses a different parsing logic than parse_datetime. The latter does dateparser.parse(date, settings={"RETURN_AS_TIMEZONE_AWARE": True}).
Why do need these extra manipulations with pub_time? I don't follow how making fields optional required it.
| self.version = None | ||
| else: | ||
| self.version = version | ||
| if len(branch) == 0 or branch == "null": |
There was a problem hiding this comment.
This logic was lost in this PR. If a string text is empty or equal to "null", we still want to treat it as empty, unless there is a reason not to.
| end_time: int, | ||
| version: Optional[str], | ||
| branch: Optional[str], | ||
| commit: Optional[str], |
There was a problem hiding this comment.
idea (maybe outside of scope of this PR): today, if we have other event tags like "environment": "prod", we'll crash with unexpected keyword argument. Maybe we can change this class such that it'd work arbitrary tags.
2 participants
#24
What
Proof it works
With the graphite example in the repo, we now see
Which makes sense as we do not set
runbut we do set everything else