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.

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

Adding dogstream filename globbing support. Fixes #753. #1550

Closed
wants to merge 1 commit into from
Closed

Adding dogstream filename globbing support. Fixes #753. #1550

wants to merge 1 commit into from

Conversation

gtaylor
Copy link

@gtaylor gtaylor commented Apr 16, 2015

We needed this in order to write dynamic Ansible roles without resorting to regex and Jinja2 hacks. This should make other people who manage Datadog config with Chef/Puppet/Ansible/Salt happy.

I broke the logic up for this path munging a bit and provided a unit test for the globbing. Sucks that I missed the window for 5.3.0, we could really use this!

Fixes #753 in the issue tracker.

@gtaylor
Copy link
Author

gtaylor commented Apr 17, 2015

There was a Python 2.6 issue with the original commit (there is no set literal in Py 2.6), but the latest failure seems to be some other unrelated psycopg2 regression from a previous commit to master.

@ssbarnea
Copy link

Not sure if this helps but as soon I discovered the pg8000 pure python postgresql driver, I never wanted to use psycopg2 anymore because the pure python one doesn't need to be compiled... on so many architectures.

@gtaylor
Copy link
Author

gtaylor commented Apr 17, 2015

I'm about 95% sure the psycopg2 failures aren't anything introduced by this pull request, so it should probably be tracked/discussed separately.

@gtaylor
Copy link
Author

gtaylor commented Apr 18, 2015

Just sent another PR that builds on this, #1557. You may want to merge that instead of this if you like what you see, though it is definitely more involved. You should be able to merge this first, then #1557 cleanly, if you'd like to go that route.

elif len(parts) >= 3:
dogstreams.append(Dogstream.init(
logger,
log_path=parts[0],
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason why we are not applying the same logic for <log path>:<module>:<parser function> dogstream configs ?

Copy link
Author

Choose a reason for hiding this comment

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

No specific reason, more so my unfamiliarity with the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the quick reply :) If you don't mind I'll rebase your PR on top of master and bring your logic to this part of the code too.

Copy link
Author

Choose a reason for hiding this comment

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

That would be awesome, Yann. Thanks!
On Jun 5, 2015 3:01 PM, "Yann" notifications@github.com wrote:

In checks/datadog.py
#1550 (comment):

  •    # Create a Dogstream object for each <dogstream value>
    
  •    for config_item in dogstreams_config.split(','):
    
  •        try:
    
  •            config_item = config_item.strip()
    
  •            parts = windows_friendly_colon_split(config_item)
    
  •            if len(parts) == 1:
    
  •                # If the dogstream includes a wildcard, we'll add every
    
  •                # matching path.
    
  •                for path in cls._get_dogstream_log_paths(parts[0]):
    
  •                    dogstreams.append(Dogstream.init(logger, log_path=path))
    
  •            elif len(parts) == 2:
    
  •                logger.warn("Invalid dogstream: %s" % ':'.join(parts))
    
  •            elif len(parts) >= 3:
    
  •                dogstreams.append(Dogstream.init(
    
  •                    logger,
    
  •                    log_path=parts[0],
    

Thanks for the quick reply :) If you don't mind I'll rebase your PR on top
of master and bring your logic to this part of the code too.


Reply to this email directly or view it on GitHub
https://github.com/DataDog/dd-agent/pull/1550/files#r31855564.

@yannmh
Copy link
Member

yannmh commented Jun 5, 2015

Thanks a lot @gtaylor ! It looks great.

It seems that the recent changes brought to our master branch introduced some conflicts with your PR. Would you mind rebasing it ?

@yannmh yannmh self-assigned this Jun 5, 2015
@yannmh yannmh added this to the 5.4.0 milestone Jun 5, 2015
@yannmh
Copy link
Member

yannmh commented Jun 8, 2015

Closing in favor of #1663. Thanks again @gtaylor

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3d166e3 on Aclima:wildcard_dogstreams into ** on DataDog:master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support * globs in dogstream file configuration
4 participants